Re: calloc -> malloc in get_data() and get_string()

[Michael McConville]

Ted Unangst wrote:
> Joerg Jung wrote:
> > > Michael McConville wrote:
> > > Relayd, httpd, and ntpd define the functions get_data() and
> > > get_string(). Both call calloc and then immediately memcpy.
> > > Calloc's zeroing isn't optimized out. These functions are called
> > > in network data paths in at least a couple places, so all this
> > > extra writing could have a meaningful performance impact.
> > 
> > I think this impact is negligible small and I believe that you can
> > not even measure it. So this change makes not so much sense to me.
> 
> I think there's no reason to be inefficient if the faster way of doing
> things is just as simple and correct.
> 
> > I wonder if using strndup() would make more sense for the first case
> > here?
> 
> Agreed on that point.

ok?


Index: sbin/iked/util.c
===
RCS file: /cvs/src/sbin/iked/util.c,v
retrieving revision 1.27
diff -u -p -r1.27 util.c
--- sbin/iked/util.c21 Aug 2015 11:59:28 -  1.27
+++ sbin/iked/util.c19 Nov 2015 02:59:16 -
@@ -636,17 +636,12 @@ char *
 get_string(uint8_t *ptr, size_t len)
 {
size_t   i;
-   char*str;
 
for (i = 0; i < len; i++)
if (!isprint(ptr[i]))
break;
 
-   if ((str = calloc(1, i + 1)) == NULL)
-   return (NULL);
-   memcpy(str, ptr, i);
-
-   return (str);
+   return strndup(ptr, i);
 }
 
 const char *
Index: usr.sbin/httpd/httpd.c
===
RCS file: /cvs/src/usr.sbin/httpd/httpd.c,v
retrieving revision 1.46
diff -u -p -r1.46 httpd.c
--- usr.sbin/httpd/httpd.c  5 Nov 2015 18:00:43 -   1.46
+++ usr.sbin/httpd/httpd.c  19 Nov 2015 02:59:19 -
@@ -831,18 +831,13 @@ char *
 get_string(uint8_t *ptr, size_t len)
 {
size_t   i;
-   char*str;
 
for (i = 0; i < len; i++)
if (!(isprint((unsigned char)ptr[i]) ||
isspace((unsigned char)ptr[i])))
break;
 
-   if ((str = calloc(1, i + 1)) == NULL)
-   return (NULL);
-   memcpy(str, ptr, i);
-
-   return (str);
+   return strndup(ptr, i);
 }
 
 void *
@@ -850,7 +845,7 @@ get_data(uint8_t *ptr, size_t len)
 {
uint8_t *data;
 
-   if ((data = calloc(1, len)) == NULL)
+   if ((data = malloc(len)) == NULL)
return (NULL);
memcpy(data, ptr, len);
 
Index: usr.sbin/ntpd/constraint.c
===
RCS file: /cvs/src/usr.sbin/ntpd/constraint.c,v
retrieving revision 1.20
diff -u -p -r1.20 constraint.c
--- usr.sbin/ntpd/constraint.c  17 Nov 2015 15:34:36 -  1.20
+++ usr.sbin/ntpd/constraint.c  19 Nov 2015 02:59:19 -
@@ -979,15 +979,10 @@ char *
 get_string(u_int8_t *ptr, size_t len)
 {
size_t   i;
-   char*str;
 
for (i = 0; i < len; i++)
if (!(isprint(ptr[i]) || isspace(ptr[i])))
break;
 
-   if ((str = calloc(1, i + 1)) == NULL)
-   return (NULL);
-   memcpy(str, ptr, i);
-
-   return (str);
+   return strndup(ptr, i);
 }
Index: usr.sbin/relayd/relayd.c
===
RCS file: /cvs/src/usr.sbin/relayd/relayd.c,v
retrieving revision 1.144
diff -u -p -r1.144 relayd.c
--- usr.sbin/relayd/relayd.c14 Oct 2015 07:58:14 -  1.144
+++ usr.sbin/relayd/relayd.c19 Nov 2015 02:59:19 -
@@ -1494,18 +1494,13 @@ char *
 get_string(u_int8_t *ptr, size_t len)
 {
size_t   i;
-   char*str;
 
for (i = 0; i < len; i++)
if (!(isprint((unsigned char)ptr[i]) ||
isspace((unsigned char)ptr[i])))
break;
 
-   if ((str = calloc(1, i + 1)) == NULL)
-   return (NULL);
-   memcpy(str, ptr, i);
-
-   return (str);
+   return strndup(ptr, i);
 }
 
 void *
@@ -1513,7 +1508,7 @@ get_data(u_int8_t *ptr, size_t len)
 {
u_int8_t*data;
 
-   if ((data = calloc(1, len)) == NULL)
+   if ((data = malloc(len)) == NULL)
return (NULL);
memcpy(data, ptr, len);
 

Re: calloc -> malloc in get_data() and get_string()

[Sebastian Benoit]

Michael McConville(mm...@mykolab.com) on 2015.10.28 12:05:24 -0400:
> Relayd, httpd, and ntpd define the functions get_data() and
> get_string(). Both call calloc and then immediately memcpy. Calloc's
> zeroing isn't optimized out. These functions are called in network data
> paths in at least a couple places, so all this extra writing could have
> a meaningful performance impact.

in relayd these functions are *not* called in the network path, they are used
when loading the (TLS) configuration data.

they dont have a performance impact.

> 
> 
> Index: relayd.c
> ===
> RCS file: /cvs/src/usr.sbin/relayd/relayd.c,v
> retrieving revision 1.144
> diff -u -p -r1.144 relayd.c
> --- relayd.c  14 Oct 2015 07:58:14 -  1.144
> +++ relayd.c  28 Oct 2015 15:57:16 -
> @@ -1501,9 +1501,10 @@ get_string(u_int8_t *ptr, size_t len)
>   isspace((unsigned char)ptr[i])))
>   break;
>  
> - if ((str = calloc(1, i + 1)) == NULL)
> + if ((str = malloc(i + 1)) == NULL)
>   return (NULL);
>   memcpy(str, ptr, i);
> + str[i] = '\0';
>  
>   return (str);
>  }
> @@ -1513,7 +1514,7 @@ get_data(u_int8_t *ptr, size_t len)
>  {
>   u_int8_t*data;
>  
> - if ((data = calloc(1, len)) == NULL)
> + if ((data = malloc(len)) == NULL)
>   return (NULL);
>   memcpy(data, ptr, len);
>  
> 

-- 

Re: calloc -> malloc in get_data() and get_string()

[Michael McConville]

Sebastian Benoit wrote:
> Michael McConville(mm...@mykolab.com) on 2015.10.28 12:05:24 -0400:
> > Relayd, httpd, and ntpd define the functions get_data() and
> > get_string(). Both call calloc and then immediately memcpy. Calloc's
> > zeroing isn't optimized out. These functions are called in network
> > data paths in at least a couple places, so all this extra writing
> > could have a meaningful performance impact.
> 
> in relayd these functions are *not* called in the network path, they
> are used when loading the (TLS) configuration data.
> 
> they dont have a performance impact.

I was speaking of all the daemons together. I figured it'd be easiest to
just change all instances.

calloc -> malloc in get_data() and get_string()

[Michael McConville]

Relayd, httpd, and ntpd define the functions get_data() and
get_string(). Both call calloc and then immediately memcpy. Calloc's
zeroing isn't optimized out. These functions are called in network data
paths in at least a couple places, so all this extra writing could have
a meaningful performance impact.

ok?


Index: relayd.c
===
RCS file: /cvs/src/usr.sbin/relayd/relayd.c,v
retrieving revision 1.144
diff -u -p -r1.144 relayd.c
--- relayd.c14 Oct 2015 07:58:14 -  1.144
+++ relayd.c28 Oct 2015 15:57:16 -
@@ -1501,9 +1501,10 @@ get_string(u_int8_t *ptr, size_t len)
isspace((unsigned char)ptr[i])))
break;
 
-   if ((str = calloc(1, i + 1)) == NULL)
+   if ((str = malloc(i + 1)) == NULL)
return (NULL);
memcpy(str, ptr, i);
+   str[i] = '\0';
 
return (str);
 }
@@ -1513,7 +1514,7 @@ get_data(u_int8_t *ptr, size_t len)
 {
u_int8_t*data;
 
-   if ((data = calloc(1, len)) == NULL)
+   if ((data = malloc(len)) == NULL)
return (NULL);
memcpy(data, ptr, len);
 

Re: calloc -> malloc in get_data() and get_string()

[Joerg Jung]

> Am 28.10.2015 um 17:05 schrieb Michael McConville :
> 
> Relayd, httpd, and ntpd define the functions get_data() and
> get_string(). Both call calloc and then immediately memcpy. Calloc's
> zeroing isn't optimized out. These functions are called in network data
> paths in at least a couple places, so all this extra writing could have
> a meaningful performance impact.

I think this impact is negligible small and I believe
that you can not even measure it.
So this change makes not so much sense to me.

I wonder if using strndup() would make more 
sense for the first case here?

> ok?
> 
> 
> Index: relayd.c
> ===
> RCS file: /cvs/src/usr.sbin/relayd/relayd.c,v
> retrieving revision 1.144
> diff -u -p -r1.144 relayd.c
> --- relayd.c14 Oct 2015 07:58:14 -1.144
> +++ relayd.c28 Oct 2015 15:57:16 -
> @@ -1501,9 +1501,10 @@ get_string(u_int8_t *ptr, size_t len)
>isspace((unsigned char)ptr[i])))
>break;
> 
> -if ((str = calloc(1, i + 1)) == NULL)
> +if ((str = malloc(i + 1)) == NULL)
>return (NULL);
>memcpy(str, ptr, i);
> +str[i] = '\0';
> 
>return (str);
> }
> @@ -1513,7 +1514,7 @@ get_data(u_int8_t *ptr, size_t len)
> {
>u_int8_t*data;
> 
> -if ((data = calloc(1, len)) == NULL)
> +if ((data = malloc(len)) == NULL)
>return (NULL);
>memcpy(data, ptr, len);
> 
> 

Re: calloc -> malloc in get_data() and get_string()

[Ted Unangst]

Joerg Jung wrote:
> 
> 
> > Am 28.10.2015 um 17:05 schrieb Michael McConville :
> > 
> > Relayd, httpd, and ntpd define the functions get_data() and
> > get_string(). Both call calloc and then immediately memcpy. Calloc's
> > zeroing isn't optimized out. These functions are called in network data
> > paths in at least a couple places, so all this extra writing could have
> > a meaningful performance impact.
> 
> I think this impact is negligible small and I believe
> that you can not even measure it.
> So this change makes not so much sense to me.

I think there's no reason to be inefficient if the faster way of doing things
is just as simple and correct.

> I wonder if using strndup() would make more 
> sense for the first case here?

Agreed on that point.