On Sat, Sep 01, 2018 at 11:31:49PM +0200, Gilles Chehade wrote:
> On Sat, Sep 01, 2018 at 09:20:59PM +0800, Michael Mikonos wrote:
> > Hello,
> >
> > Replace a malloc+strlcpy with strndup in cmdline_symset().
> > Parameter s is a "keyname=value" string and sym is the
> > "keyname" part.
> >
> > If s is "=value", sym will be an empty string.
> > The patch doesn't change this behaviour although
> > it might be undesirable to call symset() with
> > an empty string. Possibly it could also return -1
> > if len is zero. Thoughts?
> >
>
> Not opposed to the diff but at this late hour I find it easier to read
> the malloc+strlcpy and be sure there's not an off-by-one than with the
> strndup version, I'll read again tomorrow.
In my understanding the length argument of strndup(3) doesn't include
the terminating NUL character. I think the linux manual for strndup(3)
is slightly clearer on this because it has the text:
... only n bytes are copied, and a terminating null byte ('\0') is
added.
> Just wanted to remind you that this function is shared between daemons
> so this can't be an smtpd-only change :-)
>
>
> > Index: parse.y
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v
> > retrieving revision 1.218
> > diff -u -p -u -r1.218 parse.y
> > --- parse.y 25 Aug 2018 19:05:23 -0000 1.218
> > +++ parse.y 1 Sep 2018 12:42:45 -0000
> > @@ -2129,11 +2129,10 @@ cmdline_symset(char *s)
> > if ((val = strrchr(s, '=')) == NULL)
> > return (-1);
> >
> > - len = strlen(s) - strlen(val) + 1;
> > - if ((sym = malloc(len)) == NULL)
> > - errx(1, "cmdline_symset: malloc");
> > -
> > - (void)strlcpy(sym, s, len);
> > + len = strlen(s) - strlen(val);
> > + sym = strndup(s, len);
> > + if (sym == NULL)
> > + errx(1, "%s: strndup", __func__);
> >
> > ret = symset(sym, val + 1, 1);
> > free(sym);
> >
>
> --
> Gilles Chehade
>
> https://www.poolp.org @poolpOrg
