Re: psqlodbc crashes while collecting diagnostic records with SQLGetDiagRecW

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 <siwenter(at)gmail(dot)com> 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 <http://www.postgres.rocks>
>>
>>
>> On Thu, 22 Feb 2024 at 01:29, Andrey Sukhanov
>> <siwenter(at)gmail(dot)com> 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
>>