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).
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.
G'luck,
Peter
--
Peter Pentchev roam@xxxxxxxxxxx roam@xxxxxxxxxx pp@xxxxxxxxxxxx
PGP key: http://people.FreeBSD.org/~roam/roam.key.asc
Key fingerprint 2EE7 A7A5 17FC 124C F115 C354 651E EFB0 2527 DF13
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ 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
