[Libguestfs] [libnbd PATCH 2/3] api: Guarantee sanitized buffer on pread failure

[Eric Blake]

On Thu, Feb 10, 2022 at 02:51:56PM +0100, Laszlo Ersek wrote:
> On 02/09/22 23:07, Eric Blake wrote:
> > As mentioned in the previous patch, we left the state of the buffer
> > undefined if we fail pread prior to attempting NBD_CMD_READ.  Better
> > is to tweak the generator to sanitize the buffer unconditionally, as a
> > way of hardening against potential bugs in client applications that
> > fail to check for errors, but which should not be leaking
> > uninitialized data.  In the process, we can document that the contents
> > of buf are now merely unspecified, rather than undefined (valgrind
> > will no longer complain if you read buf, regardless of how nbd_pread
> > failed).
> > 
> > As always calling memset() can be a performance hit if the client also
> > sanitizes the buffer or is willing to take the audit risk, the next
> > patch will add a knob that allows the client to control when this
> > happens.
> > ---

> > 
> > +    (* Sanitize read buffers before any error is possible. *)
> > +    List.iter (
> > +      function
> > +      | BytesOut (n, count)
> > +      | BytesPersistOut (n, count) ->
> > +         pr "  memset (%s, 0, %s);\n" n count
> > +      | _ -> ()
> > +    ) args;
> > +
> >      (* Check current state is permitted for this call. *)
> >      if permitted_states <> [] then (
> >        let value = match errcode with
> 
> It tends to assist reviewers if you include a diff in the cover letter
> (or better yet, in the Notes section of the patch, suitably
> quoted/escaped) that shows the effect of the patch on the generated C
> source code.

Good idea.  I'm adding the following to the commit message:


    This patch causes changes to just the four APIs
    nbd_{aio_,}pread{,_structured}, with the generated lib/api.c changes
    looking like:
    
    | --- lib/api.c.bak       2022-02-10 08:15:05.418371357 -0600
    | +++ lib/api.c   2022-02-10 08:15:13.224372023 -0600
    | @@ -2871,6 +2871,7 @@ nbd_pread (struct nbd_handle *h, void *b
    |      debug (h, "enter: buf=<buf> count=%zu offset=%" PRIu64 " flags=0x%x", count, offset, flags);
    |    }
    |
    | +  memset (buf, 0, count);
    |    if (unlikely (!pread_in_permitted_state (h))) {
    |      ret = -1;
    |      goto out;

and similar in patch 3/3, which does

-  memset (buf, 0, count);
+  if (h->pread_initialize)
+    memset (buf, 0, count);

> 
> That said:
> 
> Acked-by: Laszlo Ersek <lersek at redhat.com>
> 
> Thanks,
> Laszlo

Thanks for the careful reviews.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org