leaked ODBC statement handles

[Terrence Enger]

I have let myself write quite a bit in-line.  Meanwhile, here are the
actual questions arising.

(*) Do you agree that ODatabaseMetaDataResultSet ctor should warn if
    it does not get a statement handle?

(*) Is there any value to a bug report at this stage?

Thanks,
Terry.


On Wed, 2012-07-18 at 05:15 +0200, Lionel Elie Mamane wrote:
> 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.

Something created a ODatabaseMetaDataResultSet.  Presumably the client
code had some intended purpose for the object.  But the statement
handle appeared in the ODBC log file only in SQLAllocHandle.  I
conclude that the client code did not do very much; as the client code
did not even free the handle, there is no sign that the client made a
deliberate decision to abandon its purpose.  Hmm, that sounds like it
could be a bug, or an opportunity for optimization, or an opportunity
for simplification.

I, too, am not sure exactly what I mean: the backtraces from
construction of the otherwise-unused ODatabaseMetaDataResultSet
objects each show scores of call levels.  Even assuming that my
hypothesized failed purpose is closer to the ctor than to main, I am
daunted.


> 
> >> 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.

Oh my.  I was wrong in so many ways ...

> 
> 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.
> 

Exactly so.  And moreover ...

(*) I confused ODatabaseMetaDataResultSet::dispose, which the dtor
    calls, with ODatabaseMetaDataResultSet::disposing, which frees the
    statement handle.  Sheesh, it's not as if the names differed in
    only one letter, or something.

(*) It was manifest in gdb that the dtor does not call disposing.  I
    was too close-minded to notice.

(*) The ODBC log files do not show any null handles.

Altogether, a bad day of work.  Sorry.

> 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?
> 

Yes, I shall do that.  It is more drastic what I was preparing to
offer in this function.

> 
> > 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.
> 
> 

The removal of m_bCloseHandle makes it much easier to understand the
code, so I no longer feel the need to invent elaborate hypotheses.
Add your skepticism to this, and I think the question is dead.

(I may hack the class to warn of an object essentially unused
throughout its lifetime, but that is a bit of ad-hocery that should
not be inflicted on anybody else.)


I am making one more gratuitous change: ODatabaseMetaDataResultSet
ctor warns if it does not get a statment handle.  The justification is
... well, I guess it's just my nast, suspicious mind.  May I have your
blessing?


Thank you for your patience with me.
Terry.