Subject: psqlodbc crashes while collecting diagnostic records with SQLGetDiagRecW

From: Andrey Sukhanov <[email-masked]>
Date: Thu, 22 Feb 2024 13:29:35 +0700

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

---

From: Dave Cramer <[email-masked]>
Date: Mon, 26 Feb 2024 15:11:14 -0500

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

---

From: Andrey Sukhanov <[email-masked]>
Date: Tue, 27 Feb 2024 13:07:47 +0700

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

---

From: Dave Cramer <[email-masked]>
Date: Tue, 27 Feb 2024 09:21:40 -0500

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-masked]> wrote:

> environ.c:249. Where it tries to memcpy from the buffer "msg" with offset
> "stapos".
>
>
> RecNumber, unsigned char * szSqlState, long * pfNativeError, unsigned char
> * szErrorMsg, short cbErrorMsgMax, short * pcbErrorMsg, unsigned short)
> line 250
> szErrorMsg, short cbErrorMsgMax, short * pcbErrorMsg, unsigned short) line
> 1626
> 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.
>

---

From: Andrey Sukhanov <[email-masked]>
Date: Wed, 28 Feb 2024 11:33:31 +0700

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) 
>     The problem is caused by signed integer overflow, which leads to
>     attempt to read from unallocated memory.
>
>     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.
>
>     "msg" is a valid pointer to "pgerror->__error_msg". "pgerror" also
>     have field "pgerror->errorsize" with value "32690".
>     "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:
>     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!SQLGetDiagRecW(short fHandleType, void *
>     handle, short iRecord, wchar_t * szSqlState, long * pfNativeError,
>     wchar_t * szErrorMsg, short cbErrorMsgMax, short * pcbErrorMsg)
>     line 234
>     That means, that the problem is caused by signed integer overflow,
>     which in turn causes memcpy to read from unallocated memory.
>     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.
>
>     project, and I don't know how to properly test psqlodbc on
>     different platforms.
>
>     Regards,
>     Andrey
>
>
>>     www.postgres.rocks <http://www.postgres.rocks>
>>
>>

---

From: Dave Cramer <[email-masked]>
Date: Wed, 28 Feb 2024 08:21:42 -0500

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-masked]> wrote:

> It seems that there won't be a problem with use of unsigned values, since
> all values that could be negative are handled.