Re: [Libguestfs] [libnbd PATCH 2/3] api: Guarantee sanitized buffer on pread failure
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(a)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