[Libguestfs] [libnbd PATCH 3/4] examples: glib-main-loop: Fix error handling

[Laszlo Ersek]

On 02/01/22 20:44, Eric Blake wrote:
> This example failed to check the *error parameter to the read and
> write completion callbacks.  What's more, the buffers start life
> uninitialized, so a failed read can end up leaking heap contents to
> the corresponding write.
> 
> Fortunately, the example program is hard-coded to two particular
> nbdkit invocations, and not usable to corrupt a user's data.  However,
> as this example is likely to be copied elsewhere, we should at least
> perform rudimentary error checking to avoid bad copy-and-paste that
> could turn into a more severe problem in code derived from this.
> Easiest for the example is to just punt and exit the process on any
> detected errors from either the source or destination.
> 
> Since the nbdkit invocations are hard-coded, testing that this patch
> works involves using a temporary patch, such as:
> 
> | diff --git i/examples/glib-main-loop.c w/examples/glib-main-loop.c
> | index 1982f941..f60a4a38 100644
> | --- i/examples/glib-main-loop.c
> | +++ w/examples/glib-main-loop.c
> | @@ -232,7 +232,8 @@ static struct NBDSource *gssrc, *gsdest;
> |  #define SIZE (1024*1024*1024)
> |
> |  static const char *src_args[] = {
> | -  "nbdkit", "-s", "--exit-with-parent", "-r", "pattern", "size=1G", NULL
> | +  "nbdkit", "-s", "--exit-with-parent", "-r", "--filter=error",
> | +  "pattern", "size=1G", "error-pread-rate=0.1", NULL
> |  };
> |
> |  static const char *dest_args[] = {
> 
> Fixes: bc3b4230 ("examples: Include an example of integrating with the glib main loop", v0.1.9)
> ---
>  examples/glib-main-loop.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/examples/glib-main-loop.c b/examples/glib-main-loop.c
> index 2df27878..1982f941 100644
> --- a/examples/glib-main-loop.c
> +++ b/examples/glib-main-loop.c
> @@ -402,6 +402,11 @@ finished_read (void *vp, int *error)
>    if (gssrc == NULL)
>      return 1; /* Nothing we can do, auto-retire the callback */
> 
> +  if (*error) {
> +    fprintf (stderr, "finished_read: read failed: %s\n", strerror (*error));
> +    exit (EXIT_FAILURE);
> +  }
> +
>    DEBUG (gssrc, "finished_read: read completed");
> 
>    assert (buffer->state == BUFFER_READING);
> @@ -448,6 +453,11 @@ finished_write (void *vp, int *error)
>    if (gsdest == NULL)
>      return 1; /* Nothing we can do, auto-retire the callback */
> 
> +  if (*error) {
> +    fprintf (stderr, "finished_write: write failed: %s\n", strerror (*error));
> +    exit (EXIT_FAILURE);
> +  }
> +
>    DEBUG (gsdest, "finished_write: write completed");
> 
>    assert (buffer->state == BUFFER_WRITING);
> 

Slightly inconsistent with

+    errno = *error;
+    perror ("failed to read");
+    exit (EXIT_FAILURE);

from the first patch, but does effectively the same thing.

Thanks,
Laszlo