Date: prev next · Thread: first prev next last
2012 Archives by date, by thread · List index


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

Context


Privacy Policy | Impressum (Legal Info) | Copyright information: Unless otherwise specified, all text and images on this website are licensed under the Creative Commons Attribution-Share Alike 3.0 License. This does not include the source code of LibreOffice, which is licensed under the Mozilla Public License (MPLv2). "LibreOffice" and "The Document Foundation" are registered trademarks of their corresponding registered owners or are in actual use as trademarks in one or more countries. Their respective logos and icons are also subject to international copyright laws. Use thereof is explained in our trademark policy.