oracle / odpi

ODPI-C: Oracle Database Programming Interface for Drivers and Applications
https://oracle.github.io/odpi/
Other
265 stars 75 forks source link

Possible memory leak on dpiStmt_getBindNames #86

Closed felipenoris closed 5 years ago

felipenoris commented 5 years ago

The API for getting bind names is:

int dpiStmt_getBindNames(dpiStmt *stmt, uint32_t *numBindNames, const char **bindNames, uint32_t *bindNameLengths)

Looking at dpiStmt.c source code, the outputs are assigned with:

bindNames[numActualBindNames] = bindNamesBuffer[i];
bindNameLengths[numActualBindNames] = bindNameLengthsBuffer[i];
*numBindNames = numActualBindNames;

Which makes me think that the caller of the function must allocate a vector of pointers for bind names, a vector of numbers for bind name lenghts, both arrays with enough capacity for holding the output, which can be determined before calling dpiStmt_getBindNames, by calling dpiStmt_getBindCount.

So, the caller must either stack allocate or heap allocate and free these arrays.

But what happens to the strings? The content of bindNamesBuffer[i] should be just a pointer to the start of a string that's allocated somewhere. Who frees this memory?

The reason for this question is a possible memory leak, which could be something I'm doing wrong, or a problem in ODPI-C.

https://github.com/felipenoris/Oracle.jl/issues/1

I'm using odpi v3.1.0.

anthony-tuininga commented 5 years ago

Which makes me think that the caller of the function must allocate a vector of pointers for bind names, a vector of numbers for bind name lenghts, both arrays with enough capacity for holding the output, which can be determined before calling dpiStmt_getBindNames, by calling dpiStmt_getBindCount.

So, the caller must either stack allocate or heap allocate and free these arrays.

Your understanding is correct. I discovered that the numBindNames parameter was defined incorrectly as simply OUT when it is in fact IN/OUT. I've corrected that in commit https://github.com/oracle/odpi/commit/34eda9d90c90176e245b0253b0d17c19b74b0b4c.

But what happens to the strings? The content of bindNamesBuffer[i] should be just a pointer to the start of a string that's allocated somewhere. Who frees this memory?

The Oracle Client library has allocated this memory and it will remain valid as long as the statement is valid.

This has been implemented in cx_Oracle and a script like the following does not leak memory at all.

import cx_Oracle

conn = cx_Oracle.connect("cx_Oracle/welcome@localhost/T183", encoding="UTF-8")

numIters = 0
while True:
    numIters += 1
    cursor = conn.cursor()
    cursor.prepare("""
            begin
                select :bind_var_one + :bind_var_two + :bind_var_three +
                        :bind_var_four + :bind_var_five + :bind_var_six +
                        :bind_var_seven + :bind_var_eight + :bind_var_nine
                from dual;
            end;""")
    bindNames = cursor.bindnames()
    if numIters % 1000 == 0:
        print(numIters, "iterations processed...")
felipenoris commented 5 years ago

It turns out the memory leak vanished. Looks like it was something else, like Julia's memory management, or even the operating system.

But, still, I think there's a bug in dpiStmt_getBindNames.

This line:

if (numActualBindNames == *numBindNames)

Should be something like

if (numActualBindNames >= *numBindNames)

I was passing numBindNames as output, so the initial value was 0, and this value does not trigger the array size too small exception.

anthony-tuininga commented 5 years ago

I don't think there is a bug, actually. numActualBindNames starts out with the value zero, so if *numBindNames == 0 and there is a bind variable available you should definitely get the error array size too small. Can you provide a code sample that demonstrates the issue?

felipenoris commented 5 years ago

@anthony-tuininga , you're right again! I was passing uninitialized memory. Sorry about that, again.

anthony-tuininga commented 5 years ago

No problem. Glad you got it figured out!