Em ter., 2 de abr. de 2024 às 18:13, Tom Lane <t...@sss.pgh.pa.us> escreveu:
> Ranier Vilela <ranier...@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, 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. 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
