Bug in Cursor_dealloc if cursor close raises an exception

[richard-reece]

trafficstars

Currently Cursor_dealloc looks like:

static void Cursor_dealloc(Cursor* cursor)
{
    if (Cursor_Validate((PyObject*)cursor, CURSOR_REQUIRE_CNXN))
    {
        closeimpl(cursor);
    }
    Py_XDECREF(cursor->inputsizes);
    PyObject_Del(cursor);
}

The issue is that closeimpl() can register an exception, which is then not flagged correctly (because destructors cannot). Python will complain, potentially somewhere random later, that SystemError: PyEval_EvalFrameEx returned a result with an error set or similar (depending on where the error is detected).

I believe the above code should be altered to include something like:

    closeimpl(cursor);
    if (PyErr_Occurred()) {
        // do something with the info about the exception that was raised, maybe dump it to stderr?
        PyErr_Clear();
    }

[v-chojas]

Do you have repro code? It seems what you're describing would only happen if SQLFreeHandle on the statement handle returns an error, in which case something has already gone horribly wrong.

[mkleehammer]

I have to agree with both. I think it might be a good idea for completeness and it is not risky, but if it does happen v-chojas is right - something has gone horribly wrong. The other places it could happen is when releasing objects via XDECREF, but if that goes wrong there is probably memory corruption and we'll be dumping stack soon.

I'd like to do it, but I'm going to hold off until the next build so I don't delay the next release any longer.

[richard-reece]

Sorry, I missed @v-chojas comment somehow. The "gone horribly wrong" is actually not all that unexpected or horrible: the error can happen if the cursor close is attempted but the database connection is lost due to a network issue.

[mkleehammer]

OK. I thought this was benign, but a simple implementation wreaked havoc. I'll look into the details of exceptions and destructors in the 5.0 Python 3 only release.