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>
---
 include/qapi/error.h           |  6 ++++++
 tests/unit/test-error-report.c | 18 ++++++++++++++++++
 util/error.c                   | 10 +++++++---
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index d798faeec3..f21a231bb1 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -519,6 +519,12 @@ static inline void error_propagator_cleanup(ErrorPropagator *prop)
 
 G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
 
+/*
+ * Special error destination to warn on error.
+ * See error_setg() and error_propagate() for details.
+ */
+extern Error *error_warn;
+
 /*
  * Special error destination to abort on error.
  * See error_setg() and error_propagate() for details.
diff --git a/tests/unit/test-error-report.c b/tests/unit/test-error-report.c
index b09650687b..54319c86c9 100644
--- a/tests/unit/test-error-report.c
+++ b/tests/unit/test-error-report.c
@@ -12,6 +12,7 @@
 #include <locale.h>
 
 #include "qemu/error-report.h"
+#include "qapi/error.h"
 
 static void
 test_error_report_simple(void)
@@ -103,6 +104,22 @@ test_error_report_timestamp(void)
 ");
 }
 
+static void
+test_error_warn(void)
+{
+    if (g_test_subprocess()) {
+        error_setg(&error_warn, "Testing &error_warn");
+        return;
+    }
+
+    g_test_trap_subprocess(NULL, 0, 0);
+    g_test_trap_assert_passed();
+    g_test_trap_assert_stderr("\
+test-error-report: warning: Testing &error_warn*\
+");
+}
+
+
 int
 main(int argc, char *argv[])
 {
@@ -116,6 +133,7 @@ main(int argc, char *argv[])
     g_test_add_func("/error-report/glog", test_error_report_glog);
     g_test_add_func("/error-report/once", test_error_report_once);
     g_test_add_func("/error-report/timestamp", test_error_report_timestamp);
+    g_test_add_func("/error-report/warn", test_error_warn);
 
     return g_test_run();
 }
diff --git a/util/error.c b/util/error.c
index 1e7af665b8..5537245da6 100644
--- a/util/error.c
+++ b/util/error.c
@@ -27,8 +27,9 @@ struct Error
 
 Error *error_abort;
 Error *error_fatal;
+Error *error_warn;
 
-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);
+    }
 }
 
 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;
 
     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 {
-- 
2.39.2

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

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

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

Stefan Berger <stefanb@linux.ibm.com> writes:

> 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.

I'll also be out starting tomorrow, until April 17.

The patch went in without my review, because I was unable to review it
in time.

As Coverity points out, it broke a design invariant.  Red "rethink this"
flag.  Can't do until I'm back.  Can't review the Stefan's proposed fix
either, because that one needs just as much thinking.

I suggest to treat &error_warn exactly like NULL for the release.

[...]

On Thu, 6 Apr 2023 at 15:13, Stefan Berger <stefanb@linux.ibm.com> wrote:
> 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.

"Not error_warn" is the common case, so it doesn't seem
great to copy the error around like that. My thoughts were
either:
 (1) error_handle() should handle all of the error cases,
like this:

    if (errp == &error_abort) {
       ...
       abort();
    }
    if (errp == &error_fatal) {
       ...
       exit(1);
    }
    if (errp = &error_warn) {
        warn_report_err(err);
    } else if (errp && !*errp) {
        *errp = err;
    } else {
        error_free(err);
    }

and delete the "set *errp" logic from the callers.

(note that error_setv() has done checks already that mean it
will always take the "(errp && !*errp)" path, so we don't need
to special-case for which caller this was.)

(2) error_handle() should return a bool to say whether it's handled
the error entirely and the callsite should do nothing further
with it.

I prefer (1) I think but would defer to people with more
experience with the Error APIs.

thanks
-- PMM

On 4/6/23 10:36, Peter Maydell wrote:
> On Thu, 6 Apr 2023 at 15:13, Stefan Berger <stefanb@linux.ibm.com> wrote:
>> 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.
> 
> "Not error_warn" is the common case, so it doesn't seem
> great to copy the error around like that. My thoughts were
> either:
>   (1) error_handle() should handle all of the error cases,
> like this:
> 
>      if (errp == &error_abort) {
>         ...
>         abort();
>      }
>      if (errp == &error_fatal) {
>         ...
>         exit(1);
>      }
>      if (errp = &error_warn) {
>          warn_report_err(err);
>      } else if (errp && !*errp) {
>          *errp = err;
>      } else {
>          error_free(err);
>      }
> 
> and delete the "set *errp" logic from the callers.


Like this?

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

@@ -76,7 +80,6 @@ static void error_setv(Error **errp,
      err->func = func;

      error_handle(errp, err);
-    *errp = err;

      errno = saved_errno;
  }
@@ -289,11 +292,6 @@ void error_propagate(Error **dst_errp, Error *local_err)
          return;
      }
      error_handle(dst_errp, local_err);
-    if (dst_errp && !*dst_errp) {
-        *dst_errp = local_err;
-    } else {
-        error_free(local_err);
-    }
  }

  void error_propagate_prepend(Error **dst_errp, Error *err,



> 
> (note that error_setv() has done checks already that mean it
> will always take the "(errp && !*errp)" path, so we don't need
> to special-case for which caller this was.)
> 
> (2) error_handle() should return a bool to say whether it's handled
> the error entirely and the callsite should do nothing further

> with it.
> 
> I prefer (1) I think but would defer to people with more
> experience with the Error APIs.
> 
> thanks
> -- PMM

Hi

On Thu, Apr 6, 2023 at 7:00 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 4/6/23 10:36, Peter Maydell wrote:
> > On Thu, 6 Apr 2023 at 15:13, Stefan Berger <stefanb@linux.ibm.com> wrote:
> >> 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.
> >
> > "Not error_warn" is the common case, so it doesn't seem
> > great to copy the error around like that. My thoughts were
> > either:
> >   (1) error_handle() should handle all of the error cases,
> > like this:
> >
> >      if (errp == &error_abort) {
> >         ...
> >         abort();
> >      }
> >      if (errp == &error_fatal) {
> >         ...
> >         exit(1);
> >      }
> >      if (errp = &error_warn) {
> >          warn_report_err(err);
> >      } else if (errp && !*errp) {
> >          *errp = err;
> >      } else {
> >          error_free(err);
> >      }
> >
> > and delete the "set *errp" logic from the callers.
>
>
> Like this?
>
> diff --git a/util/error.c b/util/error.c
> index 5537245da6..e5e247209a 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -46,6 +46,10 @@ static void error_handle(Error **errp, Error *err)
>       }
>       if (errp == &error_warn) {
>           warn_report_err(err);
> +    } else if (errp && !*errp) {
> +        *errp = err;
> +    } else {
> +        error_free(err);
>       }
>   }
>
> @@ -76,7 +80,6 @@ static void error_setv(Error **errp,
>       err->func = func;
>
>       error_handle(errp, err);
> -    *errp = err;
>
>       errno = saved_errno;
>   }
> @@ -289,11 +292,6 @@ void error_propagate(Error **dst_errp, Error *local_err)
>           return;
>       }
>       error_handle(dst_errp, local_err);
> -    if (dst_errp && !*dst_errp) {
> -        *dst_errp = local_err;
> -    } else {
> -        error_free(local_err);
> -    }
>   }
>


looks like a fix to me, send a patch?
thanks

>   void error_propagate_prepend(Error **dst_errp, Error *err,
>
>
>
> >
> > (note that error_setv() has done checks already that mean it
> > will always take the "(errp && !*errp)" path, so we don't need
> > to special-case for which caller this was.)
> >
> > (2) error_handle() should return a bool to say whether it's handled
> > the error entirely and the callsite should do nothing further
>
> > with it.
> >
> > I prefer (1) I think but would defer to people with more
> > experience with the Error APIs.
> >
> > thanks
> > -- PMM
>


-- 
Marc-André Lureau

On Thu, 6 Apr 2023 at 16:00, Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 4/6/23 10:36, Peter Maydell wrote:
> > On Thu, 6 Apr 2023 at 15:13, Stefan Berger <stefanb@linux.ibm.com> wrote:
> >> 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.
> >
> > "Not error_warn" is the common case, so it doesn't seem
> > great to copy the error around like that. My thoughts were
> > either:
> >   (1) error_handle() should handle all of the error cases,
> > like this:
> >
> >      if (errp == &error_abort) {
> >         ...
> >         abort();
> >      }
> >      if (errp == &error_fatal) {
> >         ...
> >         exit(1);
> >      }
> >      if (errp = &error_warn) {
> >          warn_report_err(err);
> >      } else if (errp && !*errp) {
> >          *errp = err;
> >      } else {
> >          error_free(err);
> >      }
> >
> > and delete the "set *errp" logic from the callers.
>
>
> Like this?
>
> diff --git a/util/error.c b/util/error.c
> index 5537245da6..e5e247209a 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -46,6 +46,10 @@ static void error_handle(Error **errp, Error *err)
>       }
>       if (errp == &error_warn) {
>           warn_report_err(err);
> +    } else if (errp && !*errp) {
> +        *errp = err;
> +    } else {
> +        error_free(err);
>       }
>   }
>
> @@ -76,7 +80,6 @@ static void error_setv(Error **errp,
>       err->func = func;
>
>       error_handle(errp, err);
> -    *errp = err;
>
>       errno = saved_errno;
>   }
> @@ -289,11 +292,6 @@ void error_propagate(Error **dst_errp, Error *local_err)
>           return;
>       }
>       error_handle(dst_errp, local_err);
> -    if (dst_errp && !*dst_errp) {
> -        *dst_errp = local_err;
> -    } else {
> -        error_free(local_err);
> -    }
>   }
>
>   void error_propagate_prepend(Error **dst_errp, Error *err,

Yes, that's what I had in mind.

-- PMM