Re: [PATCH] Fix buffer not null terminated on (ecpg lib) - Mailing list pgsql-hackers

Hi,

On 2020-04-23 14:36:15 +0900, Kyotaro Horiguchi wrote:
> At Thu, 23 Apr 2020 01:21:21 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
> > Em qua., 22 de abr. de 2020 às 23:27, Kyotaro Horiguchi <
> > horikyota.ntt@gmail.com> escreveu:
> > >
> > > -       strncpy(sqlca->sqlerrm.sqlerrmc, message,
> > > sizeof(sqlca->sqlerrm.sqlerrmc));
> > > -       sqlca->sqlerrm.sqlerrmc[sizeof(sqlca->sqlerrm.sqlerrmc) - 1] = 0;
> > > +       sqlca->sqlerrm.sqlerrmc[sizeof(sqlca->sqlerrm.sqlerrmc) - 1] =
> > > '\0';
> > > +       strncpy(sqlca->sqlerrm.sqlerrmc, message,
> > > sizeof(sqlca->sqlerrm.sqlerrmc) - 1);
> > >
> > > The existing strncpy then terminating by NUL works fine. I don't think
> > > there's any point in doing the reverse way.  Actually
> > > sizeof(sqlca->sqlerrm.sqlerrmc) - 1 is enough for the length but the
> > > existing code is not necessarily a bug.
> > >
> > Without understanding then, why Coveriy claims bug here.
>
> Well, handling non-terminated strings with str* functions are a sign
> of bug in most cases.  Coverity is very useful but false positives are
> annoying.  I wonder what if we attach Coverity annotations to such
> codes.

It might be worth doing something about this, for other reasons. We have
disabled -Wstringop-truncation in 716585235b1. But I've enabled it in my
debug build, because I find it useful. The only warning we're getting
in non-optimized builds is

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

One way we could address this is to use the 'nonstring' attribute gcc
has introduced, signalling that sqlca_t->sqlstate isn't zero
terminated. That removes the above warning.

https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#Common-Variable-Attributes

"The nonstring variable attribute specifies that an object or member declaration with type array of char, signed char,
orunsigned char, or pointer to such a type is intended to store character arrays that do not necessarily contain a
terminatingNUL. This is useful in detecting uses of such arrays or pointers with functions that expect NUL-terminated
strings,and to avoid warnings when such an array or pointer is used as an argument to a bounded string manipulation
functionsuch as strncpy. For example, without the attribute, GCC will issue a warning for the strncpy call below
becauseit may truncate the copy without appending the terminating NUL character. Using the attribute makes it possible
tosuppress the warning. However, when the array is declared with the attribute the call to strlen is diagnosed because
whenthe array doesn’t contain a NUL-terminated string the call is undefined. To copy, compare, of search non-string
characterarrays use the memcpy, memcmp, memchr, and other functions that operate on arrays of bytes. In addition,
callingstrnlen and strndup with such arrays is safe provided a suitable bound is specified, and not diagnosed. " 

I've not looked at how much work it'd be to make a recent-ish gcc not to
produce lots of false positives in optimized builds.

Greetings,

Andres Freund