Em qua., 3 de abr. de 2024 às 08:36, Ranier Vilela <ranier...@gmail.com> escreveu:
> > 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. > 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
