Re: psqlodbc crashes while collecting diagnostic records with SQLGetDiagRecW

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
>
>
> 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
>>
>