This was found by clang-tidy's
"clang-analyzer-security.insecureAPI.bzero" check.

Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
---
 src/util/virarptable.c | 2 +-
 tests/virpcimock.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util/virarptable.c b/src/util/virarptable.c
index d62de5e3dd..dac3486470 100644
--- a/src/util/virarptable.c
+++ b/src/util/virarptable.c
@@ -120,7 +120,7 @@ virArpTableGet(void)
             table->n = num + 1;
 
             addr = RTA_DATA(tb[NDA_DST]);
-            bzero(&virAddr, sizeof(virAddr));
+            memset(&virAddr, 0, sizeof(virAddr));
             virAddr.len = sizeof(virAddr.data.inet4);
             virAddr.data.inet4.sin_family = AF_INET;
             virAddr.data.inet4.sin_addr = *(struct in_addr *)addr;
diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index 4aa96cae08..f6280fc8b5 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -233,7 +233,7 @@ pci_read_file(const char *path,
     if ((fd = real_open(newpath, O_RDWR)) < 0)
         goto cleanup;
 
-    bzero(buf, buf_size);
+    memset(buf, 0, buf_size);
     if (saferead(fd, buf, buf_size - 1) < 0) {
         STDERR("Unable to read from %s", newpath);
         goto cleanup;
-- 
2.26.2

On Thu, Jan 28, 2021 at 11:24:35 +0100, Tim Wiederhake wrote:
> This was found by clang-tidy's
> "clang-analyzer-security.insecureAPI.bzero" check.

Any reasoning behind why bzero is bad?


> 
> Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
> ---
>  src/util/virarptable.c | 2 +-
>  tests/virpcimock.c     | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

On Thu, Jan 28, 2021 at 11:45:07AM +0100, Peter Krempa wrote:
> On Thu, Jan 28, 2021 at 11:24:35 +0100, Tim Wiederhake wrote:
> > This was found by clang-tidy's
> > "clang-analyzer-security.insecureAPI.bzero" check.
> 
> Any reasoning behind why bzero is bad?

Yeah, it is wierd to call this an insecure API.  If anything memset is
more dangerous because people invert the 2nd and 3rd args, resulting
in not setting any bytes at all.

None the less  bzero is deprecated, so it makes sense to use the
memset funtion in general.

> 
> 
> > 
> > Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
> > ---
> >  src/util/virarptable.c | 2 +-
> >  tests/virpcimock.c     | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Thu, Jan 28, 2021 at 10:59:41 +0000, Daniel Berrange wrote:
> On Thu, Jan 28, 2021 at 11:45:07AM +0100, Peter Krempa wrote:
> > On Thu, Jan 28, 2021 at 11:24:35 +0100, Tim Wiederhake wrote:
> > > This was found by clang-tidy's
> > > "clang-analyzer-security.insecureAPI.bzero" check.
> > 
> > Any reasoning behind why bzero is bad?
> 
> Yeah, it is wierd to call this an insecure API.  If anything memset is
> more dangerous because people invert the 2nd and 3rd args, resulting
> in not setting any bytes at all.

According to the manpage it can allegedly be optimized out:

       The  explicit_bzero()  function  performs the same task as bzero().  It
       differs from bzero() in that it guarantees that compiler  optimizations
       will  not  remove  the erase operation if the compiler deduces that the
       operation is "unnecessary".
> 
> None the less  bzero is deprecated, so it makes sense to use the
> memset funtion in general.

Yes it does, but the reason should be mentioned in the commit message.

On Thu, Jan 28, 2021 at 12:03:36PM +0100, Peter Krempa wrote:
> On Thu, Jan 28, 2021 at 10:59:41 +0000, Daniel Berrange wrote:
> > On Thu, Jan 28, 2021 at 11:45:07AM +0100, Peter Krempa wrote:
> > > On Thu, Jan 28, 2021 at 11:24:35 +0100, Tim Wiederhake wrote:
> > > > This was found by clang-tidy's
> > > > "clang-analyzer-security.insecureAPI.bzero" check.
> > > 
> > > Any reasoning behind why bzero is bad?
> > 
> > Yeah, it is wierd to call this an insecure API.  If anything memset is
> > more dangerous because people invert the 2nd and 3rd args, resulting
> > in not setting any bytes at all.
> 
> According to the manpage it can allegedly be optimized out:
> 
>        The  explicit_bzero()  function  performs the same task as bzero().  It
>        differs from bzero() in that it guarantees that compiler  optimizations
>        will  not  remove  the erase operation if the compiler deduces that the
>        operation is "unnecessary".

A compiler smart enough eliminate bzero can do also likely eliminate
memset.

> > None the less  bzero is deprecated, so it makes sense to use the
> > memset funtion in general.
> 
> Yes it does, but the reason should be mentioned in the commit message.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|