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

[Stefan Berger]

On 4/6/23 09:17, Peter Maydell wrote:

On Thu, 6 Apr 2023 at 14:16, Peter Maydell <peter.mayd...@linaro.org> wrote:

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):

...and also CID 1508179 (same issue, just one warning about the
callsite in error_setv() and one about the callsite in
error_propagate()).

thanks
-- PMM

I'll be out starting tomorrow. I don't see Marc-André online.

Would this be acceptable?
It ensures that if error_handle() returns, err has been freed.
In the other two cases a copy is being made of the Error that can then be used 
after the error_handle() call.


diff --git a/util/error.c b/util/error.c
index 5537245da6..7a2296e969 100644
--- a/util/error.c
+++ b/util/error.c
@@ -46,6 +46,8 @@ static void error_handle(Error **errp, Error *err)
     }
     if (errp == &error_warn) {
         warn_report_err(err);
+    } else {
+        error_free(err);
     }
 }

@@ -55,7 +57,7 @@ static void error_setv(Error **errp,
                        ErrorClass err_class, const char *fmt, va_list ap,
                        const char *suffix)
 {
-    Error *err;
+    Error *err, *err_bak;
     int saved_errno = errno;

     if (errp == NULL) {
@@ -75,8 +77,10 @@ static void error_setv(Error **errp,
     err->line = line;
     err->func = func;

+    err_bak = error_copy(err);
     error_handle(errp, err);
-    *errp = err;
+
+    *errp = err_bak;

     errno = saved_errno;
 }
@@ -285,14 +289,14 @@ void error_free_or_abort(Error **errp)

 void error_propagate(Error **dst_errp, Error *local_err)
 {
+    Error *local_err_bak;
     if (!local_err) {
         return;
     }
+    local_err_bak = error_copy(local_err);
     error_handle(dst_errp, local_err);
     if (dst_errp && !*dst_errp) {
-        *dst_errp = local_err;
-    } else {
-        error_free(local_err);
+        *dst_errp = local_err_bak;
     }
 }