leptonyu / odpic-raw

Haskell raw bindings to Oracle ODPI-C Library
BSD 3-Clause "New" or "Revised" License
18 stars 5 forks source link

bug: replace malloc with calloc and fix potential space leaks #20

Closed liminalisht closed 3 years ago

liminalisht commented 3 years ago

This PR is meant to address two issues.

The first issue is a potential space leak: usages of malloc were not accompanied by calls to free. This PR therefore extracts calls to malloc into the functions that call newData so that appropriate calls to free can be made.

The second issue is subtler and significantly more involved to describe, but potentially more pernicious. To be clear, replacing malloc with calloc here fixes the issue, but I intend to submit another PR soon that addresses the root cause of the issue.

The issue is as follows. In testing it was observed that queries that used parameter binding would sporadically (and eventually) fail when invoked multiple times in succession, whereas queries that involved no parameter binding would not. The culprit lay in the Storable instance for Data (in Database.Dpi.Internal), specifically the poke implementation. The underlying C struct has a field that indicates whether the value is null, and a field that contains the value itself. In the case of DataNull, isNull is correctly set to 1. However, for the other constructors, isNull is not set at all, when it should have been set to 0.

See for instance the DataBoolean case: https://github.com/leptonyu/odpic-raw/blob/master/src/Database/Dpi/Internal.chs#L648 . When poking, value is set, but not isNull: {#set Data -> value.asBoolean #} p (fromBool v). This means that when peeking, if isNull happens to be non-zero, it will be interpreted as DataNull. But when would that happen? Well, it could happen if malloc grabbed a region for you that for some reason doesn't evaluate to zero. This can be hard to reproduce, but I have observed it on several occasions.

Replacing malloc with calloc fixes the bug because all regions acquired with calloc before the call to poke have been zeroed out; hence setting isNull is no longer needed.

However, the more appropriate thing to do (in addition, perhaps, to replacing malloc with calloc) is to either (1) amend the poke implementation to {#set Data -> isNull #} p 0 in every case except for DataNull, or (2) amend the poke implementation to use functions like libDataSetBool. These C setter functions actually do this setting of isNull to zero for us: https://github.com/leptonyu/odpic-raw/blob/master/include/dpiData.c#L659

void dpiData_setBool(dpiData *data, int value)
{
    data->isNull = 0;
    data->value.asBoolean = value;
}

As mentioned then, replacing malloc with calloc fixes the issue and is probably a good thing to do anyway, but do expect a forthcoming PR that addresses the root cause of the issue.