On 20-06-10 14:22:53, 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.
HTH,
Simo.
I prefer to not look outside of the string's data into apparently
uninitialized memory. Was t->name initialized elsewhere? I would
write:
t->name[0] = 0;
strncat(t->name, name, MAX_TUBE_NAME_LEN-1);
if (strcmp(t->name, name)) {
...
strncat always writes a terminating NUL.
strcmp only looks at the string data, not past its end.
BTW, a better name for MAX_TUBE_NAME_LEN would be MAX_TUBE_NAME_SIZ.
C, bah.
--
____________________________________________________________________
TonyN.:' <mailto:tonynelson@xxxxxxxxxxxxxxxxx>
' <http://www.georgeanelson.com/>
_______________________________________________
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
