Re: [PULL v2 05/25] error: add global &error_warn destination

[Peter Maydell]

On Mon, 13 Mar 2023 at 11:47, <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> This can help debugging issues or develop, when error handling is
> introduced.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> Message-Id: <20230221124802.4103554-6-marcandre.lureau@redhat.com>

Hi; Coverity points out that this introduces a use-after-free
(CID 1507493):

> -static void error_handle_fatal(Error **errp, Error *err)
> +static void error_handle(Error **errp, Error *err)
>  {
>      if (errp == &error_abort) {
>          fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
> @@ -43,6 +44,9 @@ static void error_handle_fatal(Error **errp, Error *err)
>          error_report_err(err);
>          exit(1);
>      }
> +    if (errp == &error_warn) {
> +        warn_report_err(err);
> +    }
>  }

The old error_handle_fatal() either:
 * did not return
 * or it left the passed in 'err' alone

The new error_handle() introduces a new case, which
calls warn_report_err() and returns. warn_report_err()
prints the error and frees 'err'. Neither of the callsites
seems to expect the possibility "error_handle() returned
but 'err' is no longer valid":

>  G_GNUC_PRINTF(6, 0)
> @@ -71,7 +75,7 @@ static void error_setv(Error **errp,
>      err->line = line;
>      err->func = func;
>
> -    error_handle_fatal(errp, err);
> +    error_handle(errp, err);
>      *errp = err;

Here we stuff the now-invalid pointer into *errp
(ie into the global local_error). Probably harmless
but definitely rather odd.

>      errno = saved_errno;
> @@ -284,7 +288,7 @@ void error_propagate(Error **dst_errp, Error *local_err)
>      if (!local_err) {
>          return;
>      }
> -    error_handle_fatal(dst_errp, local_err);
> +    error_handle(dst_errp, local_err);
>      if (dst_errp && !*dst_errp) {
>          *dst_errp = local_err;
>      } else {

Here, if error_warn happens to be NULL then we'll
stuff the freed pointer into it. But if error_warn
is not NULL (eg because we've already had one use of
it) then the 'else' clause here does an error_free(local_err),
which is a double-free.

thanks
-- PMM