public inbox for [email protected]
help / color / mirror / Atom feedFrom: Dave Cramer <[email protected]>
To: Andrey Sukhanov <[email protected]>
Cc: [email protected]
Subject: Re: psqlodbc crashes while collecting diagnostic records with SQLGetDiagRecW
Date: Wed, 28 Feb 2024 08:21:42 -0500
Message-ID: <CADK3HHJXV0R0sOGGWUR+WbzNNOGJX5CYVKoy5tZZzyops_ARaA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CADK3HHJLym2DzrBkrUbLysDE9K=-73g8XtxFpMKgdaWho0Jdew@mail.gmail.com>
<[email protected]>
<CADK3HH+w+W=750pp3XwpwCiHrQCvOYhLyFZyp8EirirVLHGPrQ@mail.gmail.com>
<[email protected]>
Hi Andrey,
Probably makes sense. Not sure why the original authors used such small ints
Dave Cramer
www.postgres.rocks
On Tue, 27 Feb 2024 at 23:33, Andrey Sukhanov <[email protected]> wrote:
> Hi Dave,
>
> I think it should solve this problem. Thank you for your help.
>
> It seems that there won't be a problem with use of unsigned values, since
> all values that could be negative are handled.
> I'm not sure if "stapos" should be short. Product of 2byte*2byte
> variables could require 4 bytes to hold the value,
> if it's possible for "error->recsize" and "RecNumber" to hold big enough
> values at the same time.
> Maybe make "stapos" signed/unsigned int to make sure it won't happen?
>
> Regards,
> Andrey
>
>
> 27.02.2024 21:21, Dave Cramer wrote:
>
> Hi Andrey,
>
> Thoughts use unsigned word for lengths to avoid overflow by davecramer ·
> Pull Request #3 · postgresql-interfaces/psqlodbc (github.com)
> <https://github.com/postgresql-interfaces/psqlodbc/pull/3;
>
> Dave Cramer
> www.postgres.rocks
>
>
> On Tue, 27 Feb 2024 at 01:07, Andrey Sukhanov <[email protected]> wrote:
>
>> Hi Dave,
>>
>> Thank you for your time.
>> The problem is caused by signed integer overflow, which leads to attempt
>> to read from unallocated memory.
>>
>> I have downloaded psqlodbc 16 and debug symbols from
>> https://www.postgresql.org/ftp/odbc/versions/.
>> Source code from for psqlodbc 16 from
>> https://git.postgresql.org/git/psqlodbc.git.
>> I compiled the attached program with msvc 14 and msvc 16 compilers (same
>> results). Program should be compiled with defined "UNICODE" to use
>> functions (SQLGetDiagRecW etc.) that accept wchar.
>>
>> On windows 10 attached program crashes inside ER_ReturnError line
>> environ.c:249. Where it tries to memcpy from the buffer "msg" with offset
>> "stapos".
>>
>>
>> "msg" is a valid pointer to "pgerror->__error_msg". "pgerror" also have
>> field "pgerror->errorsize" with value "32690".
>> Offset "stapos" is calculated as "stapos = (RecNumber - 1) *
>> error->recsize;"
>> Here "error" is "pgerror", "recsize" = 99, "RecNumber" = 332.
>> "stapos" should be assigned the value of 32769 but it has type SWORD
>> (short int) (INT16_MAX is 32767).
>> Expression"if (stapos > msglen)" (here "msglen" = 32690) evaluates to
>> "false", because of signed integer overflow. It should've been "true"
>> instead, and function would've returned SQL_NO_DATA_FOUND.
>> Callstack:
>> vcruntime140.dll!memcpy() line 438
>> psqlodbc35w.dll!ER_ReturnError(PG_ErrorInfo * pgerror, short
>> RecNumber, unsigned char * szSqlState, long * pfNativeError, unsigned char
>> * szErrorMsg, short cbErrorMsgMax, short * pcbErrorMsg, unsigned short)
>> line 250
>> psqlodbc35w.dll!PGAPI_StmtError(void * hstmt, short RecNumber,
>> unsigned char * szSqlState, long * pfNativeError, unsigned char *
>> szErrorMsg, short cbErrorMsgMax, short * pcbErrorMsg, unsigned short) line
>> 1626
>> psqlodbc35w.dll!PGAPI_GetDiagRec(short) line 57
>> psqlodbc35w.dll!SQLGetDiagRecW(short fHandleType, void * handle,
>> short iRecord, wchar_t * szSqlState, long * pfNativeError, wchar_t *
>> szErrorMsg, short cbErrorMsgMax, short * pcbErrorMsg) line 234
>> odbc32.dll!SQLGetDiagRecW()
>> odbc.pg.exe!CollectDiagRecords(short hndlType, void * hndl) line 30
>> odbc.pg.exe!main(int argc, char * * argv) line 117
>>
>> That means, that the problem is caused by signed integer overflow, which
>> in turn causes memcpy to read from unallocated memory.
>> The easiest way to reproduce this problem is to try increasing the number
>> of "raise notice" messages in for loop. If their number gets past certain
>> point, no diagnostic records from SQLGetDiagRec are being returned at all.
>> Then I've decreased the number of these messages until they started
>> appearing again. In my case at this number I am able to reproduce the
>> problem. With further decrease of this number problem doesn't appear
>> anymore.
>>
>> I can't fix this problem myself, because I'm not familiar with the
>> project, and I don't know how to properly test psqlodbc on different
>> platforms.
>>
>> Regards,
>> Andrey
>>
>>
>> 27.02.2024 3:11, Dave Cramer wrote:
>>
>> Hi Andrey,
>>
>> I've tried on PostgreSQL 11 and 15 with no luck to replicate this problem.
>>
>> Is it possible for you to debug it and send more information?
>>
>> Dave Cramer
>> www.postgres.rocks
>>
>>
>> On Thu, 22 Feb 2024 at 01:29, Andrey Sukhanov <[email protected]> wrote:
>>
>>> Dear pgsql-odbc developers,
>>>
>>> Windows 10, psqlodbc 16 (psqlodbc35w.dll), postgresql 11.
>>> Getting certain amount of diagnostic records with SQLGetDiagRecW crashes
>>> the application with memory access violation.
>>>
>>> Steps to reproduce:
>>> 1. Create procedure:
>>> CREATE OR REPLACE PROCEDURE crashme()
>>> LANGUAGE plpgsql
>>> AS $$
>>> BEGIN
>>> FOR i IN 1..841 LOOP
>>> RAISE NOTICE 'msgmsgmsgmsg (%)', i;
>>> END LOOP;
>>> END; $$;
>>>
>>> 2. Use example code in attachments.
>>> 3. Application crashes with memory access violation after calling
>>> SQLGetDiagRecW function, with iRecord = 332.
>>> Application doesn't crash if number of iterations in procedure's for
>>> loop is changed.
>>> Expected outcome: SQLGetDiagRecW would return SQL_NO_DATA when there's
>>> no more diagnostic records.
>>>
>>> Regards,
>>> Andrey
>>>
>>
view thread (6+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: psqlodbc crashes while collecting diagnostic records with SQLGetDiagRecW
In-Reply-To: <CADK3HHJXV0R0sOGGWUR+WbzNNOGJX5CYVKoy5tZZzyops_ARaA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox