Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rfJsx-00GupJ-CZ for pgsql-odbc@arkaria.postgresql.org; Wed, 28 Feb 2024 13:22:12 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1rfJsv-00AZW8-1e for pgsql-odbc@arkaria.postgresql.org; Wed, 28 Feb 2024 13:22:09 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rfJsu-00AZW0-IA for pgsql-odbc@lists.postgresql.org; Wed, 28 Feb 2024 13:22:09 +0000 Received: from pgintl.fastcrypt.com ([149.56.129.164]) by makus.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1rfJso-001nkq-Ol for pgsql-odbc@lists.postgresql.org; Wed, 28 Feb 2024 13:22:07 +0000 Received: from mail-ej1-f48.google.com (mail-ej1-f48.google.com [209.85.218.48]) by pgintl.fastcrypt.com (Postfix) with ESMTPSA id 1578F202BA for ; Wed, 28 Feb 2024 08:22:01 -0500 (EST) Received: by mail-ej1-f48.google.com with SMTP id a640c23a62f3a-a293f2280c7so839696066b.1 for ; Wed, 28 Feb 2024 05:22:01 -0800 (PST) X-Gm-Message-State: AOJu0YzD3VqIwxdkZs/9H5Hfhqh+N9+kwEUimPlpN8qwzXj2+evdN4bd PXRoeySyn4xOnsI9/lo1pJGmmeeC0mKkFowBG5BWaVQdi0ZfSQ1jbRTAI3RfqdHMNuvQvx4TkhT pVvB/rD5j3FvQHCuBEie0SG1TDLI= X-Google-Smtp-Source: AGHT+IG07KRlcpsxJdLyfuLtMbXSkRBRHPtRtdHM9Aiypx8TjGp1mYWHDPx0eRSyVIojRKJ1kD5wxs61tyL1558uQb4= X-Received: by 2002:a17:906:2b05:b0:a3e:77d2:9e04 with SMTP id a5-20020a1709062b0500b00a3e77d29e04mr8635149ejg.24.1709126520833; Wed, 28 Feb 2024 05:22:00 -0800 (PST) MIME-Version: 1.0 References: <112f6883-34e4-4372-8bda-d04f45ee31bf@gmail.com> <1ad9d955-445f-4eef-a4cd-83071beba366@gmail.com> <9dc06227-b554-4dd7-ad4e-2ffbe8490d08@gmail.com> In-Reply-To: <9dc06227-b554-4dd7-ad4e-2ffbe8490d08@gmail.com> From: Dave Cramer Date: Wed, 28 Feb 2024 08:21:42 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: psqlodbc crashes while collecting diagnostic records with SQLGetDiagRecW To: Andrey Sukhanov Cc: pgsql-odbc@lists.postgresql.org Content-Type: multipart/alternative; boundary="000000000000268ef106127107de" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000268ef106127107de Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Andrey, Probably makes sense. Not sure why the original authors used such small int= s Dave Cramer www.postgres.rocks On Tue, 27 Feb 2024 at 23:33, Andrey Sukhanov 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 = =C2=B7 > Pull Request #3 =C2=B7 postgresql-interfaces/psqlodbc (github.com) > > > Dave Cramer > www.postgres.rocks > > > On Tue, 27 Feb 2024 at 01:07, Andrey Sukhanov 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 offse= t >> "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 =3D (RecNumber - 1) * >> error->recsize;" >> Here "error" is "pgerror", "recsize" =3D 99, "RecNumber" =3D 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" =3D 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 ch= ar >> * 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) li= ne >> 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 numbe= r >> of "raise notice" messages in for loop. If their number gets past certai= n >> point, no diagnostic records from SQLGetDiagRec are being returned at al= l. >> 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 proble= m. >> >> 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 wrote= : >> >>> Dear pgsql-odbc developers, >>> >>> Windows 10, psqlodbc 16 (psqlodbc35w.dll), postgresql 11. >>> Getting certain amount of diagnostic records with SQLGetDiagRecW crashe= s >>> 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 =3D 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 >>> >> --000000000000268ef106127107de Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi=C2=A0Andrey,

Probably makes=C2=A0sen= se. Not sure why the original authors used such small ints

Dave Cramer
www.postgres.rocks


On Tue, 27 Feb 2024 at 23:33, Andrey Sukhano= v <siwenter@gmail.com> wrot= e:
=20 =20 =20
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.=C2=A0=C2=A0 Pro= duct of 2byte*2byte variables could require 4 bytes to hold the value,=C2=A0
if it's possible for "error->recsize" and "RecNum= ber" to hold big enough values at the same time.
Maybe make=C2=A0"stapos" signed/unsigned int to make sure it = won't happen?

Regards,
Andrey


27.02.2024 21:21, Dave Cramer wrote:
=20
On Tue, 27 Feb 2024 at 01:07, Andrey Sukhanov <siwenter@gmail.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://w= ww.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&= quot; is a valid pointer to "pgerror->__error_msg". &q= uot;pgerror" also have field "pgerror->errorsize" with value "32690".
Offset "stapos" is calculated as "stapos =3D (RecNumb= er - 1) * error->recsize;"=C2=A0
Here "error" is "pgerror", "recsize"= =3D 99, "RecNumber" =3D 332.
"stap= os" should be assigned the value of 32769=C2=A0 but it has type SWORD (short int) (INT16_MAX is 32767).
Expression= "if (stapos > msglen)" (here "msglen" =3D 32690) evaluates to "false", because of signed integer ove= rflow.=C2=A0 It should've been "true" instead, and function = would've returned SQL_NO_DATA_FOUND.
Callstack:
=C2=A0=C2=A0=C2=A0=C2=A0 vcruntime140.dll!memcpy() line 438
=C2=A0=C2=A0=C2=A0=C2=A0 psqlodbc35w.dll!ER_ReturnError(PG_Erro= rInfo * pgerror, short RecNumber, unsigned char * szSqlState, long * pfNativeError, unsigned char * szErrorMsg, short cbErrorMsgMax, short * pcbErrorMsg, unsigned short) line 250 =C2=A0=C2=A0=C2=A0=C2=A0 psqlodbc35w.dll!PGAPI_StmtError(void *= hstmt, short RecNumber, unsigned char * szSqlState, long * pfNativeError, unsigned char * szErrorMsg, short cbErrorMsgMax, short * pcbErrorMsg, unsigned short) line 1626
=C2=A0=C2=A0=C2=A0=C2=A0 psqlodbc35w.dll!PGAPI_GetDiagRec(short= ) line 57
=C2=A0=C2=A0=C2=A0=C2=A0 psqlodbc35w.dll!SQLGetDiagRecW(short f= HandleType, void * handle, short iRecord, wchar_t * szSqlState, long * pfNativeError, wchar_t * szErrorMsg, short cbErrorMsgMax, short * pcbErrorMsg) line 234
=C2=A0=C2=A0=C2=A0=C2=A0 odbc32.dll!SQLGetDiagRecW()
=C2=A0=C2=A0=C2=A0=C2=A0 odbc.pg.exe!CollectDiagRecords(short h= ndlType, void * hndl) line 30
=C2=A0=C2=A0=C2=A0=C2=A0 odbc.pg.exe!main(int argc, char * * ar= gv) 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 f= or 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 famili= ar 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=C2=A0Andrey,

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


On Thu, 22 Feb 2024 a= t 01:29, Andrey Sukhanov <siwenter@gmail.com> wrote:
Dear pgsql-odbc developers,

Windows 10,=C2=A0 psqlodbc 16 (psqlodbc35w.dll), postgres= ql 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
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RAISE NO= TICE 'msgmsgmsgmsg (%)', i;
END LOOP;
END; $$;

2. Use example code in attachments.
3. Application crashes with memory access violation after calling
SQLGetDiagRecW function, with iRecord =3D 332.
Application doesn't crash if number of iterations in= =C2=A0 procedure's for
loop is changed.
Expected outcome: SQLGetDiagRecW would return SQL_NO_DATA when there's
no more diagnostic records.

Regards,
Andrey
--000000000000268ef106127107de--