| Message ID | 20230313114648.426607-6-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,v2,01/25] util: drop qemu_fork() | expand |
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; } }
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
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
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. [...]
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 >
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 {