Bug 112947 - Possible write to free'd memory in OResultSet.cxx
Summary: Possible write to free'd memory in OResultSet.cxx
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
5.4.2.2 release
Hardware: All All
: medium normal
Assignee: Julien Nabet
URL:
Whiteboard: target:6.0.0 target:5.4.3
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-06 17:34 UTC by Nick Gorham
Modified: 2017-10-09 06:57 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Gorham 2017-10-06 17:34:50 UTC
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;
}
Comment 1 Xisco Faulí 2017-10-06 21:20:29 UTC
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
Comment 2 Julien Nabet 2017-10-07 16:39:55 UTC
(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?
Comment 3 Lionel Elie Mamane 2017-10-07 17:38:26 UTC
(In reply to Julien Nabet from comment #2)
> Lionel: any thoughts about this?

Sounds good.
Comment 4 Julien Nabet 2017-10-07 18:10:28 UTC
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
Comment 5 Nick Gorham 2017-10-07 18:45:18 UTC
If its no trouble, then yes, please submit for me, it saves me working out what to do.
Comment 6 Nick Gorham 2017-10-07 18:47:28 UTC
> 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.
Comment 7 Julien Nabet 2017-10-07 21:02:06 UTC
I submitted a patch for master sources here:
https://gerrit.libreoffice.org/#/c/43234/
Comment 8 Commit Notification 2017-10-08 05:47:50 UTC
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.
Comment 9 Julien Nabet 2017-10-08 06:53:04 UTC
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
Comment 10 Nick Gorham 2017-10-08 10:09:20 UTC
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.
Comment 11 Julien Nabet 2017-10-08 12:04:22 UTC
(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/
Comment 12 Commit Notification 2017-10-08 17:18:14 UTC
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.
Comment 13 Commit Notification 2017-10-08 18:33:21 UTC
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.
Comment 14 Commit Notification 2017-10-09 06:54:20 UTC
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.
Comment 15 Julien Nabet 2017-10-09 06:57:27 UTC
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.