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.maydell@linaro.org> wrote:
> > 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):
>
> ...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;
     }
 }