bzero() -> explicit_bzero() in bgpd(8)

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

bzero() -> explicit_bzero() in bgpd(8)

Michael McConville-2
These seem like they were definitely meant to be explicit zeroings.


Index: pfkey.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/pfkey.c,v
retrieving revision 1.44
diff -u -p -r1.44 pfkey.c
--- pfkey.c 10 Feb 2015 05:18:39 -0000 1.44
+++ pfkey.c 10 Sep 2015 18:35:12 -0000
@@ -464,14 +464,14 @@ pfkey_reply(int sd, u_int32_t *spip)
  len = hdr.sadb_msg_len * PFKEY2_CHUNK;
  if (read(sd, data, len) != len) {
  log_warn("pfkey read");
- bzero(data, len);
+ explicit_bzero(data, len);
  free(data);
  return (-1);
  }
 
  if (hdr.sadb_msg_type == SADB_GETSPI) {
  if (spip == NULL) {
- bzero(data, len);
+ explicit_bzero(data, len);
  free(data);
  return (0);
  }
@@ -489,7 +489,7 @@ pfkey_reply(int sd, u_int32_t *spip)
  }
  }
  }
- bzero(data, len);
+ explicit_bzero(data, len);
  free(data);
  return (0);
 }

Reply | Threaded
Open this post in threaded view
|

Re: bzero() -> explicit_bzero() in bgpd(8)

Loganaden Velvindron-2
On Thu, Sep 10, 2015 at 6:36 PM, Michael McConville <
[hidden email]> wrote:

> These seem like they were definitely meant to be explicit zeroings.
>
> Hi,

I'm not entirely sure about this. Since the variable (data) is used before
return, it would not be optimized away by the compiler.

A case where optimization would happen would be:

bzero(data,len);
return (-1);


Or maybe I'm wrong here ?



>
> Index: pfkey.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/pfkey.c,v
> retrieving revision 1.44
> diff -u -p -r1.44 pfkey.c
> --- pfkey.c     10 Feb 2015 05:18:39 -0000      1.44
> +++ pfkey.c     10 Sep 2015 18:35:12 -0000
> @@ -464,14 +464,14 @@ pfkey_reply(int sd, u_int32_t *spip)
>         len = hdr.sadb_msg_len * PFKEY2_CHUNK;
>         if (read(sd, data, len) != len) {
>                 log_warn("pfkey read");
> -               bzero(data, len);
> +               explicit_bzero(data, len);
>                 free(data);
>                 return (-1);
>         }
>
>         if (hdr.sadb_msg_type == SADB_GETSPI) {
>                 if (spip == NULL) {
> -                       bzero(data, len);
> +                       explicit_bzero(data, len);
>                         free(data);
>                         return (0);
>                 }
> @@ -489,7 +489,7 @@ pfkey_reply(int sd, u_int32_t *spip)
>                         }
>                 }
>         }
> -       bzero(data, len);
> +       explicit_bzero(data, len);
>         free(data);
>         return (0);
>  }
>
>
Reply | Threaded
Open this post in threaded view
|

Re: bzero() -> explicit_bzero() in bgpd(8)

Philip Guenther-2
On Thu, Sep 10, 2015 at 8:45 PM, Loganaden Velvindron
<[hidden email]> wrote:

> On Thu, Sep 10, 2015 at 6:36 PM, Michael McConville <
> [hidden email]> wrote:
>
>> These seem like they were definitely meant to be explicit zeroings.
>>
>> Hi,
>
> I'm not entirely sure about this. Since the variable (data) is used before
> return, it would not be optimized away by the compiler.
>
> A case where optimization would happen would be:
>
> bzero(data,len);
> return (-1);
>
>
> Or maybe I'm wrong here ?

It might not be necessary from the optimization danger *right now*,
but it may in the future, and serves as documentation of why you
bothered to memset it (as opposed to doing so for some other reason,
like so it can be reused, or to avoid double free of a pointer in it,
etc).


Philip Guenther

Reply | Threaded
Open this post in threaded view
|

Re: bzero() -> explicit_bzero() in bgpd(8)

Michael McConville-2
Philip Guenther wrote:

> Loganaden Velvindron wrote:
> > Michael McConville wrote:
> >> These seem like they were definitely meant to be explicit zeroings.
> >
> > I'm not entirely sure about this. Since the variable (data) is used
> > before return, it would not be optimized away by the compiler.
> >
> > [...]
> >
> > Or maybe I'm wrong here ?

> It might not be necessary from the optimization danger *right now*,
> but it may in the future, and serves as documentation of why you
> bothered to memset it (as opposed to doing so for some other reason,
> like so it can be reused, or to avoid double free of a pointer in it,
> etc).

Thanks, this is exactly how I was thinking of it.

Anecdotally, I dumped the binary and it seems that (at least with the
system compiler) memset was being called.

Reply | Threaded
Open this post in threaded view
|

Re: bzero() -> explicit_bzero() in bgpd(8)

Claudio Jeker
In reply to this post by Michael McConville-2
On Thu, Sep 10, 2015 at 02:36:41PM -0400, Michael McConville wrote:
> These seem like they were definitely meant to be explicit zeroings.
>

OK claudio@

>
> Index: pfkey.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/pfkey.c,v
> retrieving revision 1.44
> diff -u -p -r1.44 pfkey.c
> --- pfkey.c 10 Feb 2015 05:18:39 -0000 1.44
> +++ pfkey.c 10 Sep 2015 18:35:12 -0000
> @@ -464,14 +464,14 @@ pfkey_reply(int sd, u_int32_t *spip)
>   len = hdr.sadb_msg_len * PFKEY2_CHUNK;
>   if (read(sd, data, len) != len) {
>   log_warn("pfkey read");
> - bzero(data, len);
> + explicit_bzero(data, len);
>   free(data);
>   return (-1);
>   }
>  
>   if (hdr.sadb_msg_type == SADB_GETSPI) {
>   if (spip == NULL) {
> - bzero(data, len);
> + explicit_bzero(data, len);
>   free(data);
>   return (0);
>   }
> @@ -489,7 +489,7 @@ pfkey_reply(int sd, u_int32_t *spip)
>   }
>   }
>   }
> - bzero(data, len);
> + explicit_bzero(data, len);
>   free(data);
>   return (0);
>  }
>

--
:wq Claudio