Re: Fix out-of-bounds in the function PQescapeinternal (src/interfaces/libpq/fe-exec.c)

Поиск
Список
Период
Сортировка
От Ranier Vilela
Тема Re: Fix out-of-bounds in the function PQescapeinternal (src/interfaces/libpq/fe-exec.c)
Дата
Msg-id CAEudQAq-xfcgjBEj4Gnn59+3fQVgJvVxcYdrmPRYHmFs3tw6SQ@mail.gmail.com
обсуждение исходный текст
Ответ на Re: Fix out-of-bounds in the function PQescapeinternal (src/interfaces/libpq/fe-exec.c)  (Tom Lane <tgl@sss.pgh.pa.us>)
Ответы Re: Fix out-of-bounds in the function PQescapeinternal (src/interfaces/libpq/fe-exec.c)  (Ranier Vilela <ranier.vf@gmail.com>)
Список pgsql-hackers

Em ter., 2 de abr. de 2024 às 18:13, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
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.
Yeah, the message is a little confusing.
It seems to me that it is a replacement of the malloc function with its own, with some type of security cookie,
 
> 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.
Right, the quote or backslash is counted in input_len.
In the test I did, the string had 10 quotes, so
input_len had at least 10 bytes for quotes.
But we write 20 quotes, in the slow path.

if (*s == quote_char || (!as_ident && *s == '\\'))
*rp++ = *s;
*rp++ = *s;

|0| = quote_char
|1| = quote_char
|2| = quote_char
|3| = quote_char
|4| = quote_char
|5| = quote_char
|6| = quote_char
|7| = quote_char
|8| = quote_char
|9| = quote_char
|10| = quote_char <--memory overwrite
|11| = quote_char
|12| = quote_char
|13| = quote_char
|14| = quote_char
|15| = quote_char
|16| = quote_char
|17| = quote_char
|18| = quote_char
|19| = quote_char



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.
It seems to me that only the fast path is being validated by the assert.

if (num_quotes == 0 && (num_backslashes == 0 || as_ident))
{
memcpy(rp, str, input_len);
rp += input_len;
}

best regards,
Ranier Vilela

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Alvaro Herrera
Дата:
Сообщение: Re: LogwrtResult contended spinlock
Следующее
От: Bharath Rupireddy
Дата:
Сообщение: Re: Introduce XID age and inactive timeout based replication slot invalidation