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