search

lurcher / unixODBC

The unixODBC Project goals are to develop and promote unixODBC to be the definitive standard for ODBC on non MS Windows platforms.
GNU Lesser General Public License v2.1
94 stars 51 forks source link

Update SQLCancel.c #161

Closed matthew-wozniczka closed 4 months ago

matthew-wozniczka commented 4 months ago

Refactored how SQLCancel checks for the driver returning 01S05 to be a bit easier to read, and a tiny bit faster (Can skip a call to SQLGetDiagField/SQLGetDiagFieldW)

lurcher commented 4 months ago

On 08/03/2024 23:55, Matthew Wozniczka wrote:

Refactored how |SQLCancel| checks for the driver returning 01S05 to be a bit easier to read, and a tiny bit faster (Can skip a call to |SQLGetDiagField|/|SQLGetDiagFieldW|)

    You can view, comment on, or merge this pull request online at:

https://github.com/lurcher/unixODBC/pull/161

    Commit Summary

(1 file https://github.com/lurcher/unixODBC/pull/161/files)

— Reply to this email directly, view it on GitHub https://github.com/lurcher/unixODBC/pull/161, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABYK62KYNHSUFZKHEPFBK5LYXJFX7AVCNFSM6AAAAABENTEES6VHI2DSMVQWIX3LMV43ASLTON2WKOZSGE3TMOJTGYYDEMA. You are receiving this because you are subscribed to this thread.Message ID: @.***>

That all looks perfectly good. But its got me looking at

memcmp(state, "0\0001\000S\0000\0005\0", 10)

And thinking that is making a assumption about little endian byte ordering. I know you have just taken whats there, but after accepting your change, I think I will check if that doesn't need changing.

matthew-wozniczka commented 4 months ago

Yeah actually I did notice that and think that might be a problem, but didn't wanna get into it, then forgot about even reporting it.

matthew-wozniczka commented 4 months ago

Actually... I was only looking at this file because I'm currently trying to debug a weird hang in our tests which seems to occur within the loop I modified in this pull request.

And it's on AIX, using PowerPC, a big-endian processor! And the tests which are hanging ARE testing the case where the connection has been configured as an ODBC 2.X connection, and SQLCancel is causing the cursor to be closed.

But I don't see how messing up the check like this can cause some sort of infinite loop, so I dismissed that as the cause.

matthew-wozniczka commented 4 months ago

One other thing I just noticed while staring at the loop is that it's using a SQLCHAR buffer, but passing it as SQLWCHAR* for unicode drivers.

This is actually unsafe, as there can be alignment issues, which can cause bus error signals, or at least lower performance (sometimes greatly)

It's safer to use a SQLWCHAR buffer, since that will surely be aligned for SQLCHAR (which I'm pretty sure is guaranteed to be char, and thus not need any special alignment)

matthew-wozniczka commented 4 months ago

Also, doesn't unixODBC support being compiled w/ SQLWCHAR == UTF-32? This code assumes UTF-16

matthew-wozniczka commented 4 months ago

unixODBC already has code to translate from SQLGetDiagFieldW -> SQLGetDiagField, so maybe it would be simpler to simply reuse it here and always compare against "01S05"?

lurcher commented 4 months ago

On 11/03/2024 07:12, Matthew Wozniczka wrote:

Also, doesn't unixODBC support being compiled w/ |SQLWCHAR| == |UTF-32|? This code assumes |UTF-16|

— Reply to this email directly, view it on GitHub https://github.com/lurcher/unixODBC/pull/161#issuecomment-1987759263, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABYK62JDLUZ6ZWSOT74UJZDYXVRO7AVCNFSM6AAAAABENTEES6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBXG42TSMRWGM. You are receiving this because you modified the open/close state.Message ID: @.***>

Good point about the alignment, I have just changed it to use your suggestion, re allignment. AFAIK, the buffer that's being compared is already being set in a way that should work for both 2 and 4 byte SQLWCHAR, as should the rest of the code.

lurcher commented 4 months ago

On 11/03/2024 07:15, Matthew Wozniczka wrote:

unixODBC already /has/ code to translate from |SQLGetDiagFieldW| -> |SQLGetDiagField|, so maybe it would be simpler to simply reuse it here and always compare against |"01S05"| directly?

— Reply to this email directly, view it on GitHub https://github.com/lurcher/unixODBC/pull/161#issuecomment-1987762300, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABYK62MGFQZVBA3AQXSJFYLYXVRYZAVCNFSM6AAAAABENTEES6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBXG43DEMZQGA. You are receiving this because you modified the open/close state.Message ID: @.***>

Maybe, but lets see if your search for the problem you are having unearths anything before changing something for reason of simplicity. I know its a old age thing, but as long as the "it works" part of "if it works, don't try and fix it" I like to leave well alone.

matthew-wozniczka commented 4 months ago

ah, I think I may know why I have a 'hang': When the old code was fetching the number of diagnostic records, it was using SQLULEN, but that field is documented to be SQLINTEGER.

Now, on little-endian platforms, this doesn't matter, since it initializes the variable to 0.

But on big-endian platforms, in 64-bit mode, it's incorrectly interpreting the returned value as 2^32 times MORE than what the driver actually tried to report.

That, combined with the early-exit from the loop being broken on big-endian platforms would probably result in something that I (actually our test automation) could misinterpret as a hang.

matthew-wozniczka commented 4 months ago

AFAIK, the buffer that's being compared is already being set in a way that should work for both 2 and 4 byte SQLWCHAR, as should the rest of the code.

I don't see how, as the memcmp hardcodes the comparison length to 10?

lurcher commented 4 months ago

On 11/03/2024 09:11, Matthew Wozniczka wrote:

AFAIK, the buffer that's being compared is
already being set in a way that should work for both 2 and 4 byte
SQLWCHAR, as should the rest of the code.

I don't see how, as the |memcmp| hardcodes the comparison length to 10?

— Reply to this email directly, view it on GitHub https://github.com/lurcher/unixODBC/pull/161#issuecomment-1987933214, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABYK62ONPZGFVSQLNEZ6JCTYXV7MRAVCNFSM6AAAAABENTEES6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBXHEZTGMRRGQ. You are receiving this because you modified the open/close state.Message ID: @.***>

Good spot on the original problem. But to answer the question, unless I am wrong

static SQLWCHAR str_01S05[ 5 ] = { '0', '1', 'S', '0', '5' };

Will either allocate 10 or 20 bytes depending on the size of a SQLWCHAR

SQLWCHAR state[6]

Will allocate space for 12 or 24 bytes of something

and

memcmp(state, str_01S05, sizeof( str_01S05 ))

Will compare however many bytes the SQLWCHAR was allocated as.

lurcher commented 4 months ago

On 11/03/2024 09:15, Nick Gorham wrote:

On 11/03/2024 09:11, Matthew Wozniczka wrote:

AFAIK, the buffer that's being compared is
already being set in a way that should work for both 2 and 4 byte
SQLWCHAR, as should the rest of the code.

I don't see how, as the |memcmp| hardcodes the comparison length to 10?

— Reply to this email directly, view it on GitHub https://github.com/lurcher/unixODBC/pull/161#issuecomment-1987933214, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABYK62ONPZGFVSQLNEZ6JCTYXV7MRAVCNFSM6AAAAABENTEES6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBXHEZTGMRRGQ. You are receiving this because you modified the open/close state.Message ID: @.***>

Good spot on the original problem. But to answer the question, unless I am wrong

static SQLWCHAR str_01S05[ 5 ] = { '0', '1', 'S', '0', '5' };

Will either allocate 10 or 20 bytes depending on the size of a SQLWCHAR

SQLWCHAR state[6]

Will allocate space for 12 or 24 bytes of something

and

memcmp(state, str_01S05, sizeof( str_01S05 ))

Will compare however many bytes the SQLWCHAR was allocated as.

Just to avoid confusion, how it was, was wrong as you said, my comments relate to what's there now.