MySQL Bugs: #69864: Need to add a lock to access connections member in ENV structure

Bug #69864	Need to add a lock to access connections member in ENV structure
Submitted:	29 Jul 2013 8:46	Modified:	22 Aug 2013 14:53
Reporter:	Nan Xiao	Email Updates:
Status:	Closed	Impact on me:	None
Category:	Connector / ODBC	Severity:	S1 (Critical)
Version:	5.2.5	OS:	Any
Assigned to:	Bogdan Degtyariov	CPU Architecture:	Any

Description:
In my_SQLAllocConnect function:
{
   ......
   dbc->env= penv;
   penv->connections= list_add(penv->connections,&dbc->list);
   ......
}
In my_SQLFreeConnect function:
{
   ......
   dbc->env->connections= list_delete(dbc->env->connections,&dbc->list);
   ......
}

I think we should a lock to protect connections in ENV structure. In multi-thread program, when multiple threads shares the common ENV, it is possible to cause memory access issue.

Best Regards
Nan Xiao   

How to repeat:
Multi-thread program.

Hi Nan,

Thank you for reporting the problem in Connector/ODBC driver and for your interest in MySQL software.

You are right, the env->connections list has to be properly mutex-ed in order to avoid simultaneous access to it.

Hi, Bogdan:

    Thanks for your reply!
    Could you provide a temporary patch for it? I can test for it. Thanks!

Best Regards
Nan Xiao

Hi, Bogdan:

    I don't think this is a correct fix. I think we should add a lock in ENV:
	
	typedef struct	tagENV
	{
	  SQLINTEGER   odbc_ver;
	  LIST	       *connections;
	  MYERROR      error;
	  
	  #ifdef THREAD
	  pthread_mutex_t lock;
	  #endif

	} ENV;
	
Best Regards
Nan Xiao

    I

Nan,

Sorry, it was my mistake. Thanks for pointing it out. The diff was generated from the wrong repository I experimented with. I actually added the new lock member into ENV structure just as you advised.
Uploading the correct diff...

Patch without the test case (not possible to have a reliable test case)

Attachment: bug69684.diff (application/octet-stream, text), 3.17 KiB.

Hi, Bogdan:

    OK, I will test it. But I think you should also add lock for windows code.

Best Regards
Nan Xiao

Nan,

The patch will work because pthread_mutex_lock is mapped to EnterCriticalSection() for Windows in MySQL headers.

Hi, Bogdan:
    
	Thanks very much for your patch! After testing, it seems OK!
	
	There are 2 issues:
	
	(1) From your patch, I see;
	--- driver/options.c	2013-06-28 16:53:41 +0000
	+++ driver/options.c	2013-08-01 07:37:10 +0000
	@@ -608,6 +608,7 @@
                 /* otherwise, associate this statement with the desc */
                 LIST *e= (LIST *) my_malloc(sizeof(LIST), MYF(0));
                 e->data= stmt;
+                /* No need to lock */
                 desc->exp.stmts= list_add(desc->exp.stmts, e);
				 
	I don't read the code totally, are you sure the exp.stmts member in DESC structure no need to be protected?
	
	(2) Do you have approximate date of official release of this patch?
	
	Thanks very much in advance!

Best Regards
Nan Xiao

Nan,

Q1: are you sure the exp.stmts member in DESC structure no need to be protected

A1: That particular piece of code can only be called from SQLSetStmtAttr() function. Like any other stmt-related ODBC function under no circumstances it should be asynchronously executed at the same time upon the same statement from two different threads. Otherwise, the entire function code for every function should be protected.
------------------------------------------------------------------------
Q2: Do you have approximate date of official release of this patch?

A2: Accordingly to the company policy we cannot disclose dates of the software product releases prior to the official announcement. Also, we cannot make any promises about including any particular patch into any particular version of the product. Sorry if it causes inconveniences for you.

Hi, Bogdan:

    Got it! Thanks very much again for your kindly support! You can close the issue.

Best Regards
Nan Xiao

The patch has been pushed into the source trees:

Connector/ODBC 5.1 revision 1103
Connector/ODBC 5.2 revision 1160
Connector/ODBC 5.3 revision 1172

Added the following entry to the changelog of Connector/ODBC 5.2.6 and 5.1.13: 

Added a lock in the ENV structure against simultaneous accesses to an environment handle's connection list, in order to avoid memory access issues that might occur when multiple threads share the same environment handle.