Re: leaked ODBC statement handles

On Tue, Jul 17, 2012 at 03:46:14PM -0400, Terrence Enger wrote:
> On Tue, 2012-07-17 at 19:08 +0200, Lionel Elie Mamane wrote:
> > On Mon, Jul 16, 2012 at 10:15:02PM -0400, Terrence Enger wrote:

> > > I am chasing some "leaked" ODBC statement handles.

> > > I see that ODatabaseMetaDataResultSet.cxx takes care *not* to free a
> > > statement handle which has not been subjected to one of 13 member
> > > functions with names starting "open...".  Questions arising ...

> > That is a bug. For the history, look at commit
> > aa3f42832756b7165a0722b2d013a572acf224c8
> > http://cgit.freedesktop.org/libreoffice/core/commit/?id=aa3f42832756b7165a0722b2d013a572acf224c8

> > (...) I can easily believe the code was leaking statement handles
> > in this way even back then (or

> Well, I can demonstrate at least a leak.  Removing m_bCloseHandle will
> of course fix the leak.  Still, I wonder if the leak could be a sign
> of a bug is client code somewhere.  Thoughts?

I'm not sure what you mean there.

> > double-freeing them or whatever).

> Well, yes.  I can imagine (but have not yet tried to demonstrate) this
> happening:

> (*) Client code calls close, which calls dispose, which frees the
>     statement handle

> (*) Client code calls dispose, which frees the statement handle a
>     second time.  I have not looked at the Reference class since the
>     days of OO.o, but IIRC it calls dispose of the referenced class.

> (*) The destructor calls dispose, which frees the statement handle a
>     third time..

Good point. However, calling disposing() multiple times does not lead
to double or triple freeing of the statement handle with the current
code.

ODatabaseMetaDataResultSet::disposing:

    ::osl::MutexGuard aGuard(m_aMutex);
    if(m_bFreeHandle)
        m_pConnection->freeStatementHandle(m_aStatementHandle);


So, m_pConnection->freeStatementHandle can be called multiple times,
OK. But look at OConnection::freeStatementHandle:

void OConnection::freeStatementHandle(SQLHANDLE& _pHandle)
{
    N3SQLFreeStmt(_pHandle,SQL_RESET_PARAMS);
    N3SQLFreeStmt(_pHandle,SQL_UNBIND);
    N3SQLFreeStmt(_pHandle,SQL_CLOSE);
    N3SQLFreeHandle(SQL_HANDLE_STMT,_pHandle);

    _pHandle = SQL_NULL_HANDLE;
}

1) The argument is passed by reference

2) After freeing the handle, it sets the handle to a special dummy
   value; because it is passed by reference, it is updated in the
   caller, and DatabaseMetaDataResultSet::m_aStatementHandle is set to
   the dummy value.

3) So the second time disposing() is called, SQLFreeHandle is called
   on the dummy handle, not on the original "real" handle.

The remaining doubt I could have is whether calling free on the NULL
handle is guaranteed safe; I don't see anything to that extent in the
ODBC docs, maybe it would be safer to add:

  if (_pHandle == SQL_NULL_HANDLE)
     return;

to the top of OConnection::freeStatementHandle?


> Removing m_bCloseHandle from ODatabaseMetaDataResultSet clears away
> many questions that I was accumulating, and it makes somewhat
> plausible my guess that the protocol for using
> ODatabaseMetaDataResultSet is something like this:

>     construct
>     exactly one open<whatever>
>     in any order
>      |  one or more get<whatever>
>      |  zero or more calls repositioning the result set
>      |  zero or more calls querying position of result set
>     exactly one close
>     exactly one dispose
>     destruct

All these "exactly one" are too dangerous; as far as possible, the
class should be safe for "zero or more" of anything :)

Except for things like "exactly one construct() before anything else",
which are pretty much guaranteed by C++ semantics :)

> If the guess is plausible, should the class complain (SAL_WARN_IF, I
> presume) about violations of the protocol?

If there are legitimate protocol limitations, it is good, at the very
least in debug mode, to check for protocol violations and react
accordingly; from a warning to an exception, as appropriate.

My first guess is that there are no such legitimate limitations here,
but you've looked at this code more than me; if you think there are,
let me know.

-- 
Lionel