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

[Philip Guenther]

On Thu, Sep 10, 2015 at 8:45 PM, Loganaden Velvindron
 wrote:
> On Thu, Sep 10, 2015 at 6:36 PM, Michael McConville <
> mmcco...@sccs.swarthmore.edu> 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

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

[Michael McConville]

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.

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

[Loganaden Velvindron]

On Thu, Sep 10, 2015 at 6:36 PM, Michael McConville <
mmcco...@sccs.swarthmore.edu> 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 -  1.44
> +++ pfkey.c 10 Sep 2015 18:35:12 -
> @@ -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);
>  }
>
>

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

[Michael McConville]

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 -  1.44
+++ pfkey.c 10 Sep 2015 18:35:12 -
@@ -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);
 }

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

[Claudio Jeker]

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 -  1.44
> +++ pfkey.c   10 Sep 2015 18:35:12 -
> @@ -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