Bug #46910 Mysql 5.1 + ODBC connector 5.1.5
Submitted: 25 Aug 2009 5:48 Modified: 30 Jun 2010 9:26
Reporter: Serg Laste Email Updates:
Status: Closed Impact on me:
None 
Category:Connector / ODBC Severity:S1 (Critical)
Version:5.1.5 OS:Windows (XP SP2)
Assigned to: Lawrenty Novitsky CPU Architecture:Any
Tags: BDS2007+dbGO+MySQL ODBC Driver 5.1.5

[25 Aug 2009 5:48] Serg Laste
Description:
If I disable pooling through the connection string: 'OLE DB Services=-2;' than I receive catastrophic failure (debugger moves to ntdll.DbgBreakPoint + 0x1) in the following code:
 ADOQuery.SQL.Text := 'CALL storedproc(param1);';
 ADOQuery.Open;
Without disabling pooling all works ok.
If I moove code into try-except block - no exceptions occur but debugger still jumps to ntdll.DbgBreakPoint. After that ADO components are unusable. Each and every call repeats error.

Stack:
:7c901231 ntdll.DbgBreakPoint + 0x1
:7c9551ad ; ntdll.dll
:7c969a94 ; ntdll.dll
:7c96ac57 ; ntdll.dll
:7c96ae5a ; ntdll.dll
:7c96defb ; ntdll.dll
:7c94a5d0 ; ntdll.dll
:7c9268ad ; ntdll.dll
:5b1f2345 ; C:\WINDOWS\System32\verifier.dll
:1001b181 ; C:\Program Files\MySQL\Connector ODBC 5.1\myodbc5.dll
:73fb673f ODBC32.SQLGetConnectAttrW + 0x3a
:4de76ef8 ; C:\Program Files\Common Files\System\Ole DB\msdasql.dll
:6135ad9a MSDATL3.?IsZombie@CBaseObjZombie@@QAEHXZ + 0x7e
:4de71f17 ; C:\Program Files\Common Files\System\Ole DB\msdasql.dll
:73f53777 ; C:\Program Files\Common Files\System\Ole DB\oledb32.dll
:73f40220 ; C:\Program Files\Common Files\System\Ole DB\oledb32.dll
:73f401be ; C:\Program Files\Common Files\System\Ole DB\oledb32.dll
:4dde7df3 ; C:\Program Files\Common Files\System\ado\msado15.dll
:4dde9ead ; C:\Program Files\Common Files\System\ado\msado15.dll
:4ddea2a2 ; C:\Program Files\Common Files\System\ado\msado15.dll
:4dddd73b ; C:\Program Files\Common Files\System\ado\msado15.dll
:4dddc9b4 ; C:\Program Files\Common Files\System\ado\msado15.dll
:4ddd75a3 ; C:\Program Files\Common Files\System\ado\msado15.dll
:4ddd73bd ; C:\Program Files\Common Files\System\ado\msado15.dll
:0049b361 MinRedraw + $15
:0048dc37 TDataSet.SetActive + $5B
:0048da2f TDataSet.Open + $F
:00452d9e TControl.Click + $6A
:0045679f TWinControl.WndProc + $4FF
:0043b061 TButtonControl.WndProc + $71
:004568f0 DoControlMsg + $28
:0045679f TWinControl.WndProc + $4FF
:004673d6 TCustomForm.WndProc + $536
:00455ec7 TWinControl.MainWndProc + $2F
:00428632 StdWndProc + $16
:77d38709 USER32.GetDC + 0x72

How to repeat:
add OLE DB Services=-2; to the ODBC connection string and try get result set from stored proc. For instance
storedproc(`p1` int)
BEGIN
  SELECT 0 as ret;
END

Delphi code:
 ADOQuery.SQL.Text := 'CALL storedproc(p1);';
 ADOQuery.Open;
[25 Aug 2009 6:24] Tonci Grgin
Hi Serg and thanks for your report.

I still haven't properly tested dbGO so I'd like you to attach complete sample project demonstrating this error.

Now, don't take me wrong, I'm the biggest fan of Delphi around here, but their implementation of ADO was more than problematic over the years.
I would also like to say that "pooling" is *not* implemented in c/ODBC but in DM so I expect to first find an error in Delphi - ODBC DM and only then (and maybe) in ODBC DM - c/ODBC.
[25 Aug 2009 8:25] Serg Laste
Sample project

Attachment: Projects.rar (application/x-rar-compressed, text), 2.95 KiB.

[25 Aug 2009 8:30] Serg Laste
I've added sample project - few lines of code...
I tried to trace through delphi db source code but without results. It looks like memory error somewhere I can't trace. Even exception is not generated.

PS. I am disabling connection pooling because I need immidiate failover after network error.
[25 Aug 2009 8:36] Serg Laste
You wrote:
I would also like to say that "pooling" is *not* implemented in c/ODBC but in DM so I
expect to first find an error in Delphi - ODBC DM and only then (and maybe) in ODBC DM -
c/ODBC.

But how could connection pooling be implemented in delphi DM? Where in source code could I locate it? I do not understand good ADO mechanism but as I see dbGo is just wrapper around OLE ADO..
[25 Aug 2009 8:49] Tonci Grgin
Serg, there is no Delphi DM... there is only ODBC DM(DriverManager) which is, on windows, odbcad32.exe. It provides, among other things, functions like tracing and pooling and all ODBC calls go through it.
There are known SW products that disable tracing functionality of DM, like IIS, so to confirm this you better start machine wide tracing in DM (see out manual if you don't know how to do it), then start your app and stop tracing after closing the application. Attach that log as it might prove useful.
[25 Aug 2009 8:57] Serg Laste
I tried tracing. There is nothing special in ODBC tracing SQL.log file.
[25 Aug 2009 9:02] Tonci Grgin
Serg, if it's *DM* trace, please attach it. If c/ODBC is indeed failing it should be visible there.
[25 Aug 2009 9:45] Tonci Grgin
Using only ODBC calls, I have no problems...

	Env. Attr. SQL_ATTR_ODBC_VERSION set to SQL_OV_ODBC3

	Successfully connected to DSN '5-1-5-x64-on-opensol'.
CALL spbug46910_1() (no params as none is needed)

SQLExecDirect:	In: hstmt = 0x00000000001FD980, szSqlStr = "", cbSqlStr = -3
		Return:	SQL_SUCCESS=0

Get Data All:
"ret"
1
1 row fetched from 1 column.
[25 Aug 2009 9:48] Tonci Grgin
Redoing your SP as follows:
DELIMITER $$

DROP PROCEDURE IF EXISTS `spbug46910_1` $$
CREATE DEFINER=`root`@`%` PROCEDURE `spbug46910_1`()
BEGIN
  SELECT 1 AS ret;
END $$

DELIMITER ;

yields correct result in my Delphi2007 without ";OLE DB Services=-2;" in connection string...
[25 Aug 2009 9:49] Tonci Grgin
Although it works with ";OLE DB Services=-2;" too.
[25 Aug 2009 10:31] Serg Laste
I begin to suspect that I suffer from OS problems... I'll try to reinstall.

But does it work with OLE DB Service=-2 when several Fetches are made one after one? I mean: ADOQuery.Open; ADOQuery.Close; ADOQuery.Open.
[25 Aug 2009 10:55] Tonci Grgin
Yes it does, I even changed ret value in between calls... Still working.
[25 Aug 2009 12:21] Serg Laste
Weird!
I *totally* reinstalled windows xp sp2. Than installed Mysql 5.1 than MySQL ODBC driver 5.1.5. and BDS2007
-------------------

Error is in its place. When I use connection string without OLE DB Services=-2; - all is ok. But when I try to add this param debugger goes to ntdll.DbgBreakPoint - and all crashes. Crash occures at second call to ADOQuery.CALL;

I attached trace log. And I see only one error:
SQLGetConnectAttrW with return code -1 (SQL_ERROR)
[25 Aug 2009 12:23] Serg Laste
ODBC trace log

Attachment: SQL.LOG (application/octet-stream, text), 213.06 KiB.

[25 Aug 2009 12:35] Serg Laste
Could you please give your connection string with disble pooling parameter.
[25 Aug 2009 12:48] Serg Laste
This strange behavior occurs only on CALL proc; statements. I can succesfully open any standard query "SELECT * FROM ...;".
And one more note:
1. I rebuild connection string and try to run CALL proc; - all works ok
2. I add OLE DB Services=-2; - try open - fault.
3. If now I remove OLE DB Service=-2 the result don't change - faults,faults...

so conclusion 1 - all problems occur only for stored procs. 2 - need to rebuild connection string fully (after crash I begin use Mysql ODBC 3.5 connector instead of 5.1.5).
[26 Aug 2009 14:18] Serg Laste
??? Situation didn't change...
[27 Aug 2009 7:43] Tonci Grgin
Serg, I did not change your sample at all, so connection string is the same as yours...

I do not know what to do here as I can not "fix" what I see as functioning. I'll do one more test today and review your trace.
[27 Aug 2009 9:48] Tonci Grgin
There seems to be a genuine problem here which I need to check on.
Back to "Analyzing".
[31 Aug 2009 11:45] Tonci Grgin
Yes, this is a problem.

Code breaks right after entering utility.c, my_bool reget_current_catalog... with observation that dbc->database is FFFFFFFFFF... while dbc->mysql->db is "test" which is correct in my case.

I'll try reproducing the problem without Delphi now.
[31 Aug 2009 13:45] Tonci Grgin
Finally, we came to, what seems to be, the bottom of this problem..

message = 0x0016fd46 "[MySQL][ODBC 5.1 Driver]Commands out of sync; you can't run this command now"
happens on select database() from reget_current_catalog. After that the dbc->database is corrupted.

Verified as described.
[2 Sep 2009 19:42] Serg Laste
So is there any possibility to solve the problem ? ))
[3 Sep 2009 6:09] Tonci Grgin
Serg, as you can see, I did not just let go of the report. Triage and Severity are quite severe and I assigned Lawrin to fix it. But it appears, for now, not to be trivial so I do not know when the patch will be ready.
[12 Oct 2009 15:53] Lawrenty Novitsky
Patch(same as proposed for pre-review but with traditional fix of few unrelated warnings about stary variables in tests)

Attachment: 46910.diff (text/x-diff), 3.01 KiB.

[12 Oct 2009 15:55] Lawrenty Novitsky
Checking as already reviewed by Jim (have his approval email of this patch)
[10 Jun 2010 15:40] Lawrenty Novitsky
The Bug#48406 has been marked as duplicate of this bug.
Bogdan, please give this review a higher priority.
[10 Jun 2010 16:06] Lawrenty Novitsky
Bug#54404 is marked as duplicate too
[10 Jun 2010 19:42] Lawrenty Novitsky
Bug#11588 is marked as duplicate of this one.
[10 Jun 2010 20:17] Lawrenty Novitsky
continuation of discussion started at Bug#48406.

As for the correctness of the patch. My patch does pretty much the same that you/Emmanuel Kartmann propose.
"Do not free memory twice. A simple fix would be to NULLify the pointer after free and checking if it's not NULL before released again:"

That's exactly what my patch does. It NULLs database pointer in only really possible case when its value won't be changed to a new value after freeing previously pointed memory. Maybe it would be even more reliable if nulling moved outside the "if" to the right after freeing memory... but i don't see it's really needed.
[10 Jun 2010 21:58] Edwin van Putten
> Maybe it would be even more reliable if nulling moved outside the "if" to the right after freeing memory... but i don't see it's really needed.

I think it's best to zero memory that you free *right away*. There's no point in keeping that now defunct pointer any longer.

And suppose what happens if the function changes or just doesn't work the way we expect (dives into the ELSE block). We can avoid all that trouble by just moving that NULL assignment up.

Now that the patch wasn't accepted (yet?) anyway, we can just send a new patch with the proposed change. Let's not be lazy and prove them we can still actually do this simple programming task ;-)
[10 Jun 2010 22:01] Edwin van Putten
Hmm I noticed my english has quit a few bugs as well ;-)
[11 Jun 2010 6:59] Lawrenty Novitsky
Edwin,

If function doesn't work the expected way - execution continues in "if" block and database gets NULL. if everything's fine and and "else" block is executed - database gets new value. and if smth is going wrong in else block - most probably things are so bad, that assigning null to database won't really help :)

Reviewer has powers to tell to move that assigning statement up for those couple of lines. i don't mind to do that on my side.
[11 Jun 2010 11:45] Edwin van Putten
Yes, you are right - the patch will work :). But being pedantic about pointer issues like this rewards itself now and then... 

I'd appreciate it if there's a new version of the connector that has been patched with the aforementioned diff file (or a slightly modified one)...

Right now we have MySQL servers reaching their max_connections limit of 100, because we cannot release 'errored' connections. We avoid disconnecting errored connections on purpose: to avoid the crashes due to this issue...
[15 Jun 2010 8:33] Lawrenty Novitsky
I think i'm not allowed or at least not encouraged to spend my time on building hotfixes for someone who is not entitled for them. But you can always pull our latest sources from our open repository(look for it on launchpad dot net, you will need bzr), apply patch and build binaries. later you can substitute that your custom build with our official release that contains the patch.
[17 Jun 2010 9:55] Lawrenty Novitsky
the patch has been pushed to the trunk as rev#898 and will go to the next release.

tony: here is how i described th bug in the ChangeLog:
"Retrieving of current catalog at the moment when connection is not ready for
that(broken, not all pending results processed) leads to application crash."
I hope that can help to understand what's this bug about. or you can look at duplicates.
besides i hope you will tell me is mth is wrong with my text in the changelog :)
[30 Jun 2010 9:26] Tony Bedford
An entry has been added to the 5.1.7 changelog:

Retrieval of the current catalog at the moment when a connection was not ready, such as when the connection had been broken or when not all pending results had been processed, resulted in the application crashing.