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

[Stefan Berger <stefanb@linux.ibm.com>]

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;
      }
  }