From: Tom Lane <tgl@sss.pgh.pa.us>
To: Ranier Vilela <ranier.vf@gmail.com>
cc: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Fix out-of-bounds in the function PQescapeinternal (src/interfaces/libpq/fe-exec.c)
In-reply-to: <CAEudQAokoJFsNoDwOwbuLzBKP67nMhkL0Qe_1WztEfK1JAKDCg@mail.gmail.com>
References: <CAEudQAokoJFsNoDwOwbuLzBKP67nMhkL0Qe_1WztEfK1JAKDCg@mail.gmail.com>
Comments: In-reply-to Ranier Vilela <ranier.vf@gmail.com>
	message dated "Sat, 30 Mar 2024 08:36:55 -0300"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1835779.1712092319.0@sss.pgh.pa.us>
Date: Tue, 02 Apr 2024 17:13:20 -0400
Message-ID: <1836134.1712092400@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/1836134.1712092400%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1835779.1712092319.1@sss.pgh.pa.us>

Ranier Vilela <ranier.vf@gmail.com> writes:
> While I working in [1], Coverity reported some errors:
> src/bin/pg_basebackup/pg_createsubscriber.c
> CID 1542690: (#1 of 2): Out-of-bounds access (OVERRUN)
> alloc_strlen: Allocating insufficient memory for the terminating null of
> the string. [Note: The source code implementation of the function has been
> overridden by a builtin model.]
> CID 1542690: (#2 of 2): Out-of-bounds access (OVERRUN)
> alloc_strlen: Allocating insufficient memory for the terminating null of
> the string. [Note: The source code implementation of the function has been
> overridden by a builtin model.]

Yeah, we saw that in the community run too.  I'm tempted to call it
an AI hallucination.  The "Note" seems to mean that they're not
actually analyzing our code but some made-up substitute.

> The source of errors is the function PQescapeInternal.
> The slow path has bugs when num_quotes or num_backslashes are greater than
> zero.
> For each num_quotes or num_backslahes we need to allocate two more.

Nonsense.  The quote or backslash is already counted in input_len,
so we need to add just one more.

If there were anything wrong here, I'm quite sure our testing under
e.g. valgrind would have caught it years ago.  However, just to be
sure, I tried adding an Assert that the allocated space is filled
exactly, as attached.  It gets through check-world just fine.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="assert-PQescapeInternal-isnt-broken.patch";
	charset="us-ascii"
Content-ID: <1835779.1712092319.2@sss.pgh.pa.us>
Content-Description: assert-PQescapeInternal-isnt-broken.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec=
.c
index c02a9180b2..43a4ce0458 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -4255,7 +4255,9 @@ PQescapeInternal(PGconn *conn, const char *str, size=
_t len, bool as_ident)
 =

 	/* Closing quote and terminating NUL. */
 	*rp++ =3D quote_char;
-	*rp =3D '\0';
+	*rp++ =3D '\0';
+
+	Assert(rp =3D=3D result + result_size);
 =

 	return result;
 }

------- =_aaaaaaaaaa0--