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.
> > > Finally, they also now support socket activation. Which means I was
> > > looking at .service file which I had setup to use a sysconfig
> > > environment file. When I looked at it, I realized that the default was
> > > to listen to 0.0.0.0 which means someone could install and if not
> > > really careful / pre config or what not have a service listening where
> > > they maybe don't want to. I was thinking of changing this to 127.0.0.1
> > > with a note about how it should be modified. However I figure if I do
> > > that, anyone with this installed and unmodified who gets the update
> > > will suddenly have their service stop listening to the outside world.
> > > What should be done then?
> > >
> > > My thought is I leave it as 0.0.0.0 for all released versions (but what
> > > about epel7) and then in rawhide change to 127.0.0.1. I just don't like
> > > leaving the package like that for epel as well.
> > >
> > > Any ideas/direction would be appreciated.
> > >
> > > Sincerely,
> > > --
> > > Nathanael
> > >
> > > _______________________________________________
> > > 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
> >
> > _______________________________________________
> > 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
--
Simo Sorce
RHEL Crypto Team
Red Hat, Inc
_______________________________________________
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
