On 10/06/20 21:43 +0300, Peter Pentchev wrote:
On Wed, Jun 10, 2020 at 02:22:53PM -0400, Simo Sorce wrote:
On Wed, 2020-06-10 at 14:16 -0400, Simo Sorce wrote:
> On Wed, 2020-06-10 at 20:01 +0200, Dan Čermák wrote:
> > "Nathanael D. Noblet" <nathanael@xxxxxxx> writes:
> >
> > > Hello,
> > >
> > > I maintain beanstalkd which is a message server of sorts. It recently
> > > released a new version however some changes I'm not 100% sure about.
> > >
> > > When compiling I got the following GCC error.
> > >
> > > usr/include/bits/string_fortified.h:106:10: error: '__builtin_strncpy'
> > > specified bound 201 equals destination size [-Werror=stringop-
> > > truncation]
> > > 106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos
> > > (__dest));
> > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ~~~~~~~~
> > >
> > > Results via google says that strncpy needs to have the third argument
> > > less than the 2nd, so I've got this patch, similar to others:
> > >
> > > --- beanstalkd-1.12/tube.c.org 2020-06-10 09:37:35.129224346 -0600
> > > +++ beanstalkd-1.12/tube.c 2020-06-10 09:37:42.761138035 -0600
> > > @@ -12,7 +12,7 @@
> > > if (!t)
> > > return NULL;
> > >
> > > - strncpy(t->name, name, MAX_TUBE_NAME_LEN);
> > > + strncpy(t->name, name, MAX_TUBE_NAME_LEN-1);
> > > if (t->name[MAX_TUBE_NAME_LEN - 1] != '\0') {
> > > t->name[MAX_TUBE_NAME_LEN - 1] = '\0';
> > > twarnx("truncating tube name");
> > >
> > > You'll notice it was already checking the len-1 for null. Can anyone
> > > verify that my change won't cause some un-intended bug I don't
> > > understand?
> >
> > If I understand it correctly, then you are now invoking undefined
> > behavior: you copy at most MAX_TUBE_NAME_LEN-1 bytes to t->name and then
> > check whether the following byte (that was never written to) is not
> > equal to 0. I have not checked the code of beanstalkd, but the contents
> > of t->name[MAX_TUBE_NAME_LEN - 1] will be zero if it was allocated via
> > calloc() at best or random at worst. Irrespectively of that, the check
> > now no longer does what it *should*: null terminate the string if it is
> > not null terminated. It will now randomly null terminate it or not at
> > all.
>
> strncpy() is a truly awful API unfortunately, the change is
> meaningless, but it is not random as Dan says.
>
> The original form is more correct though, because now you can get
> spurious warnings that the string have been truncated when that is not
> true. (If t->name is not zeroized you will get the spurious warning for
> any string shorter than MAX_TUBE_NAME_LEN, which is the opposite of
> what the warning says.
>
> The third argument to strncpy can be as long as the size of the buffer
> you are writing into, the additional check you have there insures that
> the string is terminated (even if that requires truncation).
>
> So I would say you should drop your change and stop believing in random
> google results :)
Sorry, hit send prematurely.
If you really want to avoid the warning instead of ignoring it, you
should change the code this way:
strncpy(t->name, name, MAX_TUBE_NAME_LEN-1);
if (t->name[MAX_TUBE_NAME_LEN - 2] != '\0') {
t->name[MAX_TUBE_NAME_LEN - 1] = '\0';
twarnx("truncating tube name");
}
NOTE: the -2 in the condition, this is needed because memory locations
start counting from 0, so if you write N-1 bytes the Nth-1 byte is at
the N-2 position when you start counting from 0.
And that is the last position your strncpy wrote to, and if that is not
0 then potentially string truncation occurred.
You still want to zero the last available byte in that case and not the
N-1 available byte, so you set N-1 to 0, not N-2.
N-1 is the Nth byte when you start counting from 0 and N is the size of
the array.
...or use snprintf() and check! its! return! value! (sorry, I've had to
explain that to too many people over too many years).
^ This.
Something like (completely, totally untested, but should give you an
idea):
const int res = snprintf(t->name, MAX_TUBE_NAME_LEN, "%s", name);
if (res < 0 || (size_t)res >= MAX_TUBE_NAME_LEN)
twarnx("truncating tube name");
...although, in all fairness, the two conditions should be checked
separately, since the first one (res < 0) indicates a serious error and
no meaningful characters at all written to t->name.
_______________________________________________
devel mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxx
Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: https://lists.fedoraproject.org/archives/list/devel@xxxxxxxxxxxxxxxxxxxxxxx
