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

Em qua., 3 de abr. de 2024 às 08:36, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
escreveu:

>
> Em ter., 2 de abr. de 2024 às 18:13, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
> escreveu:
>
>> Ranier Vilela <ranier(dot)vf(at)gmail(dot)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,
> like /GS (Buffer Security Check)
> <https://learn.microsoft.com/en-us/cpp/build/reference/gs-buffer-security-check?view=msvc-170>
>
>
>> > 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.
>
Sorry, some kind of brain damage.
I ran the test with a debugger, and step by step, the defect does not occur
in the section I indicated.
Only the exact bytes counted from quote_char and num_backslashes are
actually written.

>
> 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.
>
The assert works fine.

The only catch is Coverity will continue to report the error.
But in this case, the error does not exist and the warning is wrong.

I will remove the patch.

best regards,
Ranier Vilela