Bug #48491 Connector/C++ corrupts memory
Submitted: 3 Nov 2009 0:10 Modified: 16 Nov 2009 12:02
Reporter: Jean-Denis Muys Email Updates:
Status: Closed Impact on me:
None 
Category:Connector / C++ Severity:S1 (Critical)
Version:1.05 OS:MacOS (10.5.8 and 10.6.1)
Assigned to: Lawrenty Novitsky CPU Architecture:Any
Tags: auto_ptr, C++, Connector/C++, double free, memory corruption

[3 Nov 2009 0:10] Jean-Denis Muys
Description:
I have been having some nasty memory corruption issues in my code using Connector/C++.

While I can't exclude bugs in my code, I found one code example in Connector/C++ that was the direct cause of one of my memory corruption crashes:

Consider a call to:

sql::ResultSet * MySQL_Prepared_Statement::executeQuery()

It constructs a MySQL_Prepared_ResultSet through this constructor:

MySQL_Prepared_ResultSet::MySQL_Prepared_ResultSet(
			MYSQL_STMT * s,
			MySQL_ResultBind * r_bind,
			sql::ResultSet::enum_type rset_type,
			MySQL_Prepared_Statement * par,
			sql::mysql::util::my_shared_ptr< MySQL_DebugLogger > * l
		)

Now pay attention to second argument to that constructor: MySQL_ResultBind * r_bind.

The constructor doesn't know that this pointer is *not* his to manage, because the caller has already put it into an auto_ptr:

sql::ResultSet * MySQL_Prepared_Statement::executeQuery()
{
[...]
	std::auto_ptr< MySQL_ResultBind > result_bind(new MySQL_ResultBind(stmt, logger));
	
	sql::ResultSet * tmp = new MySQL_Prepared_ResultSet(stmt, result_bind.get(), tmp_type, this, logger);

[...]
}

Yet the constructor *also* puts it into an auto_ptr:

MySQL_Prepared_ResultSet::MySQL_Prepared_ResultSet(
	[...]
			MySQL_ResultBind * r_bind,
	[...]

		)
	: result_bind(r_bind)
[...]

So now at this point, the *same* pointer is inside *two* auto_ptr. This is a recipe for disaster. And disaster occurs, because, still in the constructor, an exception can occur. For me it once occurred on this call:

MySQL_Prepared_ResultSet::MySQL_Prepared_ResultSet([...])
	: [...]
{
[...]
	result_bind->bindResult();
[...]
}

So now, an exception in a constructor means the partially constructed object is destructed (thanks Jens). So our nice result_bind auto_ptr is destructed, which means the r_bind object is destructed too.

Now the caller has a *bad* auto_ptr pointing to a destructed object. That auto_ptr being an automatic variable, it is destructed too, and boom, r_bind's destructor is called twice on the same object.

Then all bets are off. Memory corruption follows.

Note that without an exception, the caller handles the situation:

sql::ResultSet * MySQL_Prepared_Statement::executeQuery()
{
[...]
	std::auto_ptr< MySQL_ResultBind > result_bind(new MySQL_ResultBind(stmt, logger));
	
	sql::ResultSet * tmp = new MySQL_Prepared_ResultSet(stmt, result_bind.get(), tmp_type, this, logger);
	result_bind.release();
[...]
}

But that call to release is too late. In the short time while the *same* pointer is in *two* auto_ptr, hell can break loose, and Murphy ensures that it will.

My discovery of that bad code pattern in Connector/C++ makes me very wary of using it.

I really hope my ringing this alarm call will be answered.

Jean-Denis

How to repeat:
Call executeQuery on a prepared statement that has no result, for example an UPDATE.

Suggested fix:
don't put a pointer into two auto_ptr. As a rule, don't store the result of auto_ptr::get() anywhere: neither in an object, nor pass it as an argument. Also possibly, avoid using auto_ptr as class members.

This implies a heavy code review of Connector/C++
[3 Nov 2009 18:42] Lawrenty Novitsky
Hi Jean-Denis,

I've saw your messages to the maillist.
So the bug report isn't quite new for me. And looks like you did great job in tracking it down.
The problem codehas been already changed in our repository. And instead of std::auto_ptr it's boost::shared_ptr what is used. So, lookse like the problem will be solved in the next release (if you agree that boost::shared_ptr will fix the bug).
[4 Nov 2009 7:31] Tonci Grgin
Assigning to Lawrentiy.
[16 Nov 2009 12:02] Lawrenty Novitsky
The condition that can cause this unwanted behavior has been fixed in latest sources.