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:
Category:Connector / ODBC Severity:S1 (Critical)
Version:5.2.5 OS:Any
Assigned to: Bogdan Degtyariov CPU Architecture:Any

[29 Jul 2013 8:46] Nan Xiao
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.
[30 Jul 2013 9:15] Bogdan Degtyariov
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.
[30 Jul 2013 9:18] Nan Xiao
Hi, Bogdan:

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

Best Regards
Nan Xiao
[1 Aug 2013 5:46] 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;

	} ENV;
Best Regards
Nan Xiao

[1 Aug 2013 6:25] Bogdan Degtyariov

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...
[1 Aug 2013 8:38] Bogdan Degtyariov
Patch without the test case (not possible to have a reliable test case)

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

[1 Aug 2013 9:58] Nan Xiao
Hi, Bogdan:

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

Best Regards
Nan Xiao
[1 Aug 2013 10:32] Bogdan Degtyariov

The patch will work because pthread_mutex_lock is mapped to EnterCriticalSection() for Windows in MySQL headers.
[2 Aug 2013 8:41] Nan Xiao
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
[2 Aug 2013 9:05] Bogdan Degtyariov

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.
[2 Aug 2013 9:17] Nan Xiao
Hi, Bogdan:

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

Best Regards
Nan Xiao
[14 Aug 2013 3:07] Bogdan Degtyariov
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
[22 Aug 2013 14:53] Daniel So
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.