Re: smtpd: malloc+strlcpy -> strndup
On Mon, Sep 03, 2018 at 11:43:02PM +0800, Michael Mikonos wrote:
> On Mon, Sep 03, 2018 at 02:24:49PM +0800, Michael Mikonos wrote:
> > 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 :-)
>
> Thanks for the reminder. Here is a new version of the patch to include
> other daemons. I also followed a suggestion from halex@ to remove the
> strlen() calls and determine length using val-s. Did I miss anything?
>
this looks good to me
> Index: acme-client/parse.y
> ===
> RCS file: /cvs/src/usr.sbin/acme-client/parse.y,v
> retrieving revision 1.29
> diff -u -p -u -r1.29 parse.y
> --- acme-client/parse.y 3 Aug 2018 17:57:21 - 1.29
> +++ acme-client/parse.y 3 Sep 2018 15:18:23 -
> @@ -839,17 +839,12 @@ cmdline_symset(char *s)
> {
> char*sym, *val;
> int ret;
> - size_t len;
>
> if ((val = strrchr(s, '=')) == NULL)
> return -1;
> -
> - len = strlen(s) - strlen(val) + 1;
> - if ((sym = malloc(len)) == NULL)
> - errx(EXIT_FAILURE, "cmdline_symset: malloc");
> -
> - strlcpy(sym, s, len);
> -
> + sym = strndup(s, val - s);
> + if (sym == NULL)
> + errx(EXIT_FAILURE, "%s: strndup", __func__);
> ret = symset(sym, val + 1, 1);
> free(sym);
>
> Index: bgpd/parse.y
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
> retrieving revision 1.331
> diff -u -p -u -r1.331 parse.y
> --- bgpd/parse.y 27 Aug 2018 19:32:37 - 1.331
> +++ bgpd/parse.y 3 Sep 2018 15:18:24 -
> @@ -3145,17 +3145,12 @@ cmdline_symset(char *s)
> {
> char*sym, *val;
> int ret;
> - size_t len;
>
> if ((val = strrchr(s, '=')) == NULL)
> return (-1);
> -
> - len = strlen(s) - strlen(val) + 1;
> - if ((sym = malloc(len)) == NULL)
> - fatal("cmdline_symset: malloc");
> -
> - strlcpy(sym, s, len);
> -
> + sym = strndup(s, val - s);
> + if (sym == NULL)
> + fatal("%s: strndup", __func__);
> ret = symset(sym, val + 1, 1);
> free(sym);
>
> Index: dvmrpd/parse.y
> ===
> RCS file: /cvs/src/usr.sbin/dvmrpd/parse.y,v
> retrieving revision 1.36
> diff -u -p -u -r1.36 parse.y
> --- dvmrpd/parse.y11 Jul 2018 07:39:22 - 1.36
> +++ dvmrpd/parse.y3 Sep 2018 15:18:24 -
> @@ -834,17 +834,12 @@ cmdline_symset(char *s)
> {
> char*sym, *val;
> int ret;
> - size_t len;
>
> if ((val = strrchr(s, '=')) == NULL)
> return (-1);
> -
> - len = strlen(s) - strlen(val) + 1;
> - if ((sym = malloc(len)) == NULL)
> - errx(1, "cmdline_symset: malloc");
> -
> - strlcpy(sym, s, len);
> -
> + sym = strndup(s, val - s);
> + if (sym == NULL)
> + errx(1, "%s: strndup", __func__);
> ret = symset(sym, val + 1, 1);
> free(sym);
>
> Index: eigrpd/parse.y
> ===
> RCS file: /cvs/src/usr.sbin/eigrpd/parse.y,v
> retrieving revision 1.27
> diff -u -p -u -r1.27 parse.y
> --- eigrpd/parse.y11 Jul 2018 07:39:22 - 1.27
> +++ eigrpd/parse.y3 Sep 2018 15:18:24 -
> @@ -1094,17 +1094,12 @@ cmdline_symset(char *s)
> {
> char*sym, *val;
> int ret;
> - size_t len;
>
> if ((val = strrchr(s, '=')) == NULL)
> return (-1);
> -
> - len = strlen(s) - strlen(val) + 1;
> - if ((sym = malloc(len)) == NULL)
> - errx(1, "cmdline_symset: malloc");
> -
> - strlcpy(sym, s, len);
> -
> + sym = strndup(s, val - s);
> + if (sym == NULL)
> +
Re: smtpd: malloc+strlcpy -> strndup
On Mon, Sep 03, 2018 at 02:24:49PM +0800, Michael Mikonos wrote:
> 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 :-)
Thanks for the reminder. Here is a new version of the patch to include
other daemons. I also followed a suggestion from halex@ to remove the
strlen() calls and determine length using val-s. Did I miss anything?
Index: acme-client/parse.y
===
RCS file: /cvs/src/usr.sbin/acme-client/parse.y,v
retrieving revision 1.29
diff -u -p -u -r1.29 parse.y
--- acme-client/parse.y 3 Aug 2018 17:57:21 - 1.29
+++ acme-client/parse.y 3 Sep 2018 15:18:23 -
@@ -839,17 +839,12 @@ cmdline_symset(char *s)
{
char*sym, *val;
int ret;
- size_t len;
if ((val = strrchr(s, '=')) == NULL)
return -1;
-
- len = strlen(s) - strlen(val) + 1;
- if ((sym = malloc(len)) == NULL)
- errx(EXIT_FAILURE, "cmdline_symset: malloc");
-
- strlcpy(sym, s, len);
-
+ sym = strndup(s, val - s);
+ if (sym == NULL)
+ errx(EXIT_FAILURE, "%s: strndup", __func__);
ret = symset(sym, val + 1, 1);
free(sym);
Index: bgpd/parse.y
===
RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
retrieving revision 1.331
diff -u -p -u -r1.331 parse.y
--- bgpd/parse.y27 Aug 2018 19:32:37 - 1.331
+++ bgpd/parse.y3 Sep 2018 15:18:24 -
@@ -3145,17 +3145,12 @@ cmdline_symset(char *s)
{
char*sym, *val;
int ret;
- size_t len;
if ((val = strrchr(s, '=')) == NULL)
return (-1);
-
- len = strlen(s) - strlen(val) + 1;
- if ((sym = malloc(len)) == NULL)
- fatal("cmdline_symset: malloc");
-
- strlcpy(sym, s, len);
-
+ sym = strndup(s, val - s);
+ if (sym == NULL)
+ fatal("%s: strndup", __func__);
ret = symset(sym, val + 1, 1);
free(sym);
Index: dvmrpd/parse.y
===
RCS file: /cvs/src/usr.sbin/dvmrpd/parse.y,v
retrieving revision 1.36
diff -u -p -u -r1.36 parse.y
--- dvmrpd/parse.y 11 Jul 2018 07:39:22 - 1.36
+++ dvmrpd/parse.y 3 Sep 2018 15:18:24 -
@@ -834,17 +834,12 @@ cmdline_symset(char *s)
{
char*sym, *val;
int ret;
- size_t len;
if ((val = strrchr(s, '=')) == NULL)
return (-1);
-
- len = strlen(s) - strlen(val) + 1;
- if ((sym = malloc(len)) == NULL)
- errx(1, "cmdline_symset: malloc");
-
- strlcpy(sym, s, len);
-
+ sym = strndup(s, val - s);
+ if (sym == NULL)
+ errx(1, "%s: strndup", __func__);
ret = symset(sym, val + 1, 1);
free(sym);
Index: eigrpd/parse.y
===
RCS file: /cvs/src/usr.sbin/eigrpd/parse.y,v
retrieving revision 1.27
diff -u -p -u -r1.27 parse.y
--- eigrpd/parse.y 11 Jul 2018 07:39:22 - 1.27
+++ eigrpd/parse.y 3 Sep 2018 15:18:24 -
@@ -1094,17 +1094,12 @@ cmdline_symset(char *s)
{
char*sym, *val;
int ret;
- size_t len;
if ((val = strrchr(s, '=')) == NULL)
return (-1);
-
- len = strlen(s) - strlen(val) + 1;
- if ((sym = malloc(len)) == NULL)
- errx(1, "cmdline_symset: malloc");
-
- strlcpy(sym, s, len);
-
+ sym = strndup(s, val - s);
+ if (sym == NULL)
+ errx(1, "%s: strndup", __func__);
ret = symset(sym, val + 1, 1);
free(sym);
Index: httpd/parse.y
===
RCS file: /cvs/src/usr.sbin/httpd/parse.y,v
retrieving revision 1.105
diff -u
Re: smtpd: malloc+strlcpy -> strndup
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 - 1.218
> > +++ parse.y 1 Sep 2018 12:42:45 -
> > @@ -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
Re: smtpd: malloc+strlcpy -> strndup
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. 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 - 1.218 > +++ parse.y 1 Sep 2018 12:42:45 - > @@ -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
smtpd: malloc+strlcpy -> strndup
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? - Michael 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 - 1.218 +++ parse.y 1 Sep 2018 12:42:45 - @@ -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);
