I noticed while writing an ODBC driver and running with valgrind I noticed that creating a DB query could lead to the driver writing to free'd space. Process is in OResultSet.cxx Row status array is allocated and set in the driver m_pRowStatusArray = new SQLUSMALLINT[1]; // the default value setStmtOption<SQLUSMALLINT*, SQL_IS_POINTER>(SQL_ATTR_ROW_STATUS_PTR, m_pRowStatusArray); However in the destructor, when OResultSet is released, m_pRowStatusArray is released, but the address is not reset in the ODBC driver, so next time that statement is used, that address is referenced. A simple (and seems to work) fix is to reset the value when its released. OResultSet::~OResultSet() { setStmtOption<SQLUSMALLINT*, SQL_IS_POINTER>(SQL_ATTR_ROW_STATUS_PTR, NULL); delete [] m_pRowStatusArray; delete m_pSkipDeletedSet; }
Dear Nick, Thanks for the plausible fix. Could you please create a patch and submit it to gerrit as described here: https://wiki.documentfoundation.org/Development/gerrit/SubmitPatch? Core developers can review your patch there
(In reply to Nick Gorham from comment #0) > ... > A simple (and seems to work) fix is to reset the value when its released. > > OResultSet::~OResultSet() > { > setStmtOption<SQLUSMALLINT*, SQL_IS_POINTER>(SQL_ATTR_ROW_STATUS_PTR, > NULL); > delete [] m_pRowStatusArray; > delete m_pSkipDeletedSet; > } m_pSkipDeletedSet is a unique_ptr so it'll be automatically destroyed in the destructor of OResultSet see https://opengrok.libreoffice.org/xref/core/connectivity/source/inc/odbc/OResultSet.hxx#138 About m_pRowStatusArray, it's also a unique_ptr see https://opengrok.libreoffice.org/xref/core/connectivity/source/inc/odbc/OResultSet.hxx#141 so will be automatically destroyed. About instruction with setStmtOption, it may help but am not sure. Lionel: any thoughts about this?
(In reply to Julien Nabet from comment #2) > Lionel: any thoughts about this? Sounds good.
Thank you Lionel Nick: if you prefer I submit a patch by quoting you (for any reason), don't hesitate to tell me. Waiting for your feedback
If its no trouble, then yes, please submit for me, it saves me working out what to do.
> m_pSkipDeletedSet is a unique_ptr so it'll be automatically destroyed in the > destructor of OResultSet > see > https://opengrok.libreoffice.org/xref/core/connectivity/source/inc/odbc/ > OResultSet.hxx#138 > Yep, thats all understood, but the ODBC driver has no way of knowing that that memory is now free, so will continue to write into it.
I submitted a patch for master sources here: https://gerrit.libreoffice.org/#/c/43234/
Julien Nabet committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=12d5e57dcac22c288ef23075b82e3e3e87929912 tdf#112947: fix write to free'd memory (odbc) It will be available in 6.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Here's the patch for 5.4 waiting for review here: https://gerrit.libreoffice.org/#/c/43236/ I noticed several uses of setStmtOption (see https://opengrok.libreoffice.org/search?project=core&q=setStmtOption&defs=&refs=&path=&hist=&type=) In OResultSet.cxx, we have too setStmtOption<SQLLEN*, SQL_IS_POINTER>(SQL_ATTR_FETCH_BOOKMARK_PTR, reinterpret_cast<SQLLEN*>(aBookmark.getArray())); should we also put this one to nullptr? Also, still in OResulSet.cxx, we have setStmtOption<SQLULEN, SQL_IS_UINTEGER>(SQL_ATTR_CURSOR_TYPE, _par0); (in setFetchDirection()) Should we put 0 (or other by default value) or since it's just an integer, we don't care? Finally the other file location for setStmtOption is OStatement.cxx
Yes, if there is a chance that the SQL_ATTR_FETCH_BOOKMARK_PTR address will remain in the driver beyond the life of the memory it points to. The cursor type is benign, as its not likely to cause a problem. I looked at the use opf SetStmtAttr in OStatement.cxx and assumed that he underlying SQL_STATEMENT would be released at the point the object was released, so there was no chance for problems. The basic problem is unlike JDBC, ODBC does not have a separate ResultSet handle, its just the Statement being in that state, so its intentional that any changes to the Statement will outlive the presence of a result wet.
(In reply to Nick Gorham from comment #10) > Yes, if there is a chance that the SQL_ATTR_FETCH_BOOKMARK_PTR address will > remain in the driver beyond the life of the memory it points to. I submitted this patch https://gerrit.libreoffice.org/#/c/43241/
Julien Nabet committed a patch related to this issue. It has been pushed to "libreoffice-5-4": http://cgit.freedesktop.org/libreoffice/core/commit/?id=01ba3b43c16e2570022f20dbdf958a091a1fef0b&h=libreoffice-5-4 tdf#112947: fix write to free'd memory (odbc) It will be available in 5.4.3. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Julien Nabet committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=b76a087f73cdf3d1a8bcc56b1ed7a5959ca0f1bd Related tdf#112947: another fix about odbc It will be available in 6.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Lionel Elie Mamane committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=2399cf59fc0467028e7183877582821dc878120f Revert "Related tdf#112947: another fix about odbc" It will be available in 6.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Let's put this one to FIXED. If there's another thing to fix (it seems still to be confirmed), please create a new bugtracker.