Обсуждение: may be a buffer overflow problem

> On 14 Jun 2024, at 09:38, Winter Loo <winterloo@126.com> wrote:

> I find the definition of `sqlca->sqlstate` and it has only 5 bytes. When the statement
>
> ```c
> strncpy(sqlca->sqlstate, "YE001", sizeof(sqlca->sqlstate));
> ```
>
> get executed, `sqlca->sqlstate` will have no '\0' byte which makes me anxious when someone prints that as a string.

sqlstate is defined as not being unterminated fixed-length, leaving the callers
to handle termination.

> Indeed, I found the code(in src/interfaces/ecpg/ecpglib/misc.c) does that,
>
> fprintf(debugstream, "[NO_PID]: sqlca: code: %ld, state: %s\n",
> sqlca->sqlcode, sqlca->sqlstate);

This is indeed buggy and need to take the length into account, as per the
attached.  This only happens when in the undocumented regression test debug
mode which may be why it's gone unnoticed.

--
Daniel Gustafsson

On Fri, 2024-06-14 at 15:38 +0800, Winter Loo wrote:
> I am using gcc version 11.3.0 to compile postgres source code. Gcc complains about the following line:
>
>   strncpy(sqlca->sqlstate, "YE001", sizeof(sqlca->sqlstate));
>
> with error as:
>
> misc.c:529:17: error: ‘strncpy’ output truncated before terminating nul
> copying 5 bytes from a string of the same length [-Werror=stringop-truncation]
>
> I find the definition of `sqlca->sqlstate` and it has only 5 bytes. When the statement
>
>   strncpy(sqlca->sqlstate, "YE001", sizeof(sqlca->sqlstate));
>
> get executed, `sqlca->sqlstate` will have no '\0' byte which makes me anxious
> when someone prints that as a string. Indeed, I found the code(in src/interfaces/ecpg/ecpglib/misc.c) does that,
>
>         fprintf(debugstream, "[NO_PID]: sqlca: code: %ld, state: %s\n",
>                 sqlca->sqlcode, sqlca->sqlstate);
>
> Is there any chance to fix the code?

I agree that that is wrong.

We could either use memcpy() to avoid the warning and use a format string
with %.*s in fprintf(), or we could make the "sqlstate" one byte longer.

I think that the second option would be less error-prone.

Yours,
Laurenz Albe

On Fri, 2024-06-14 at 09:55 +0200, Daniel Gustafsson wrote:
> > On 14 Jun 2024, at 09:38, Winter Loo <winterloo@126.com> wrote:
>
> > I find the definition of `sqlca->sqlstate` and it has only 5 bytes. When the statement
> >
> > ```c
> > strncpy(sqlca->sqlstate, "YE001", sizeof(sqlca->sqlstate));
> > ```
> >
> > get executed, `sqlca->sqlstate` will have no '\0' byte which makes me anxious when someone prints that as a string.
>
> sqlstate is defined as not being unterminated fixed-length, leaving the callers
> to handle termination.
>
> > Indeed, I found the code(in src/interfaces/ecpg/ecpglib/misc.c) does that,
> >
> > fprintf(debugstream, "[NO_PID]: sqlca: code: %ld, state: %s\n",
> > sqlca->sqlcode, sqlca->sqlstate);
>
> This is indeed buggy and need to take the length into account, as per the
> attached.  This only happens when in the undocumented regression test debug
> mode which may be why it's gone unnoticed.

So you think we should ignore that compiler warning?
What about using memcpy() instead of strncpy()?

Yours,
Laurenz Albe

> On 14 Jun 2024, at 10:06, Laurenz Albe <laurenz.albe@cybertec.at> wrote:

> So you think we should ignore that compiler warning?

We already do using this in meson.build:

  # Similarly disable useless truncation warnings from gcc 8+
  'format-truncation',
  'stringop-truncation',

--
Daniel Gustafsson

On Fri, 2024-06-14 at 10:10 +0200, Daniel Gustafsson wrote:
> > On 14 Jun 2024, at 10:06, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> > So you think we should ignore that compiler warning?
>
> We already do using this in meson.build:
>
>   # Similarly disable useless truncation warnings from gcc 8+
>   'format-truncation',
>   'stringop-truncation',

Right; and I see that -Wno-stringop-truncation is also set if you build
with "make".  So your patch is good.

I wonder how Winter Loo got to see that warning...

Yours,
Laurenz Albe

> On 14 Jun 2024, at 10:29, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> On Fri, 2024-06-14 at 10:10 +0200, Daniel Gustafsson wrote:
>>> On 14 Jun 2024, at 10:06, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>>
>>> So you think we should ignore that compiler warning?
>>
>> We already do using this in meson.build:
>>
>>   # Similarly disable useless truncation warnings from gcc 8+
>>   'format-truncation',
>>   'stringop-truncation',
>
> Right; and I see that -Wno-stringop-truncation is also set if you build
> with "make".  So your patch is good.

Thanks for looking!  I will apply it backpatched all the way down as this has
been wrong since 2006.

> I wonder how Winter Loo got to see that warning...

And it would be interesting to know if that was the only warning, since error.c
in ECPG performs the exact same string copy.

--
Daniel Gustafsson

Daniel Gustafsson <daniel@yesql.se> writes:
> This is indeed buggy and need to take the length into account, as per the
> attached.  This only happens when in the undocumented regression test debug
> mode which may be why it's gone unnoticed.

Seeing that this code is exercised thousands of times a day in the
regression tests and has had a failure rate of exactly zero (and
yes, the tests do check the output), there must be some reason
why it's okay.

            regards, tom lane

I wrote:
> Seeing that this code is exercised thousands of times a day in the
> regression tests and has had a failure rate of exactly zero (and
> yes, the tests do check the output), there must be some reason
> why it's okay.

After looking a little closer, I think the reason why it works in
practice is that there's always a few bytes of zero padding at the
end of struct sqlca_t.

I don't see any value in changing individual code sites that are
depending on that, because there are surely many more, both in
our own code and end users' code.  What I suggest we ought to do
is formalize the existence of that zero pad.  Perhaps like this:

     char        sqlstate[5];
+    char        sqlstatepad;     /* nul terminator for sqlstate */
 };

Another way could be to change

-     char        sqlstate[5];
+     char        sqlstate[6];

but I fear that could have unforeseen consequences in code that
is paying attention to sizeof(sqlstate).

Either way there are probably doc adjustments to be made.

            regards, tom lane

> On 14 Jun 2024, at 17:18, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
>> Seeing that this code is exercised thousands of times a day in the
>> regression tests and has had a failure rate of exactly zero (and
>> yes, the tests do check the output), there must be some reason
>> why it's okay.
>
> After looking a little closer, I think the reason why it works in
> practice is that there's always a few bytes of zero padding at the
> end of struct sqlca_t.
>
> I don't see any value in changing individual code sites that are
> depending on that, because there are surely many more, both in
> our own code and end users' code.  What I suggest we ought to do
> is formalize the existence of that zero pad.  Perhaps like this:
>
> char sqlstate[5];
> + char sqlstatepad;     /* nul terminator for sqlstate */
> };
>
> Another way could be to change
>
> - char sqlstate[5];
> + char sqlstate[6];
>
> but I fear that could have unforeseen consequences in code that
> is paying attention to sizeof(sqlstate).

Since sqlca is, according to our docs, present in other database systems we
should probably keep it a 5-char array for portability reasons.  Adding a
padding character should be fine though.

The attached adds padding and adjust the tests and documentation to match.  I
kept the fprintf using %.*s to match other callers.  I don't know ECPG well
enough to have strong feelings wrt this being the right fix or not, and the age
of incorrect assumptions arounf that fprintf hints at this not being a huge
problem in reality (should still be fixed of course).

--
Daniel Gustafsson

Hi,

On 2024-06-17 23:52:54 +0200, Daniel Gustafsson wrote:
> Since sqlca is, according to our docs, present in other database systems we
> should probably keep it a 5-char array for portability reasons.  Adding a
> padding character should be fine though.

How about, additionally, adding __attribute__((nonstring))? Wrapped in an
attribute, of course.  That'll trigger warning for many unsafe uses, like
strlen().

It doesn't quite detect the problematic case in ecpg_log() though, seems it
doesn't understand fprintf() well enough (it does trigger in simple printf()
cases, because they get reduced to puts(), which it understands).


Adding nonstring possibly allow us to re-enable -Wstringop-truncation, it triggers a
bunch on

../../../../../home/andres/src/postgresql/src/interfaces/ecpg/ecpglib/misc.c: In function ‘ECPGset_var’:
../../../../../home/andres/src/postgresql/src/interfaces/ecpg/ecpglib/misc.c:575:17: warning: ‘__builtin_strncpy’
outputtruncated before terminating nul copying 5 bytes from a string of the same length [-Wstringop-truncation]
 
  575 |                 strncpy(sqlca->sqlstate, "YE001", sizeof(sqlca->sqlstate));


The only other -Wstringop-truncation warnings are in ecpg tests and at least
the first one doesn't look bogus:

../../../../../home/andres/src/postgresql/src/interfaces/ecpg/test/compat_oracle/char_array.pgc: In function 'main':
../../../../../home/andres/src/postgresql/src/interfaces/ecpg/test/compat_oracle/char_array.pgc:54:5: warning:
'__builtin_strncpy'output truncated before terminating nul copying 5 bytes from a string of the same length
[-Wstringop-truncation]
   54 |     strncpy(shortstr, ppppp, sizeof shortstr);

Which seems like a valid complaint, given that shortstr is a char[5], ppppp
is "XXXXX" and thatshortstr is printed:
    printf("\"%s\": \"%s\"  %d\n", bigstr, shortstr, shstr_ind);


Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2024-06-17 23:52:54 +0200, Daniel Gustafsson wrote:
>> Since sqlca is, according to our docs, present in other database systems we
>> should probably keep it a 5-char array for portability reasons.  Adding a
>> padding character should be fine though.

> How about, additionally, adding __attribute__((nonstring))? Wrapped in an
> attribute, of course.  That'll trigger warning for many unsafe uses, like
> strlen().

What I was advocating for is that we make it *safe* for strlen, not
that we double down on awkward, non-idiomatic, unsafe coding
practices.

Admittedly, I'm not sure how we could persuade compilers that
a char[5] followed by a char field is a normal C string ...

            regards, tom lane

Hi,

On 2024-06-17 22:42:41 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2024-06-17 23:52:54 +0200, Daniel Gustafsson wrote:
> >> Since sqlca is, according to our docs, present in other database systems we
> >> should probably keep it a 5-char array for portability reasons.  Adding a
> >> padding character should be fine though.
> 
> > How about, additionally, adding __attribute__((nonstring))? Wrapped in an
> > attribute, of course.  That'll trigger warning for many unsafe uses, like
> > strlen().
> 
> What I was advocating for is that we make it *safe* for strlen, not
> that we double down on awkward, non-idiomatic, unsafe coding
> practices.

Given that apparently other platforms have it as a no-trailing-zero-byte
"string", I'm not sure that that is that clearly a win. Also, if they just
copy the field onto the stack or such, they'll have the same issue again.

And then there is this:

> Admittedly, I'm not sure how we could persuade compilers that
> a char[5] followed by a char field is a normal C string ...

I think the explicit backstop of a zero byte is a good idea, but I don't think
we'd just want to rely on it.

Greetings,

Andres Freund

On 18.06.24 04:35, Andres Freund wrote:
> On 2024-06-17 23:52:54 +0200, Daniel Gustafsson wrote:
>> Since sqlca is, according to our docs, present in other database systems we
>> should probably keep it a 5-char array for portability reasons.  Adding a
>> padding character should be fine though.
> 
> How about, additionally, adding __attribute__((nonstring))? Wrapped in an
> attribute, of course.  That'll trigger warning for many unsafe uses, like
> strlen().
> 
> It doesn't quite detect the problematic case in ecpg_log() though, seems it
> doesn't understand fprintf() well enough (it does trigger in simple printf()
> cases, because they get reduced to puts(), which it understands).

See also <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115513>.

> Adding nonstring possibly allow us to re-enable -Wstringop-truncation,

Note that that would only work because we now always use our own 
snprintf(), which is not covered by that option.  I mean, we could still 
do it, but it's not like the reasons we originally disabled that option 
have actually gone away.