On 11/09/2017 23:13, Eric Blake wrote: > We already have several files that knowingly require assert() > to work, sometimes because refactoring the code for proper > error handling has not been tackled yet; there are probably > other files that have a similar situation but with no comments > documenting the same. In fact, we have places in migration > that handle untrusted input with assertions, where disabling > the assertions risks a worse security hole than the current > behavior of losing the guest to SIGABRT when migration fails > because of the assertion. Promote our current per-file > safety-valve to instead be project-wide, and expand it to also > cover glib's g_assert(). > > Note that we do NOT want to encourage 'assert(side-effects);' > (that is a bad practice that prevents copy-and-paste of code to > other projects that CAN disable assertions; plus it costs > unnecessary reviewer mental cycles to remember whether a project > special-cases the crippling of asserts); and we would LIKE to > fix migration to not rely on asserts (but that takes a big code > audit). But in the meantime, we DO want to send a message > that anyone that disables assertions has to tweak code in order > to compile, making it obvious that they are taking on additional > risk that we are not going to support. At the same time, leave > comments mentioning NDEBUG in files that we know still need to > be scrubbed, so there is at least something to grep for. > > It would be possible to come up with some other mechanism for > doing runtime checking by default, but which does not abort > the program on failure, while leaving side effects in place > (unlike how crippling assert() avoids even the side effects), > perhaps under the name q_verify(); but it was not deemed worth > the effort (developers should not have to learn a replacement > when the standard C macro works just fine, and it would be a lot > of churn for little gain). The patch specifically uses #error > rather than #warn so that a user is forced to tweak the header > to acknowledge the issue, even when not using a -Werror > compilation. > > Signed-off-by: Eric Blake <ebl...@redhat.com> > Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org> > Reviewed-by: Thomas Huth <th...@redhat.com> > > --- > v2: promote from RFC, leave NDEBUG mentioned in comments, beef up > commit message > > I'm not sure if this would fit best as a top-level patch applied > directly by Peter, or if it would fit through qemu-trivial, or if > it belongs to Paolo's misc queue. But consensus seemed to be that > we are okay with having it. > > --- > include/qemu/osdep.h | 16 ++++++++++++++++ > hw/scsi/mptsas.c | 6 ++---- > hw/virtio/virtio.c | 6 ++---- > 3 files changed, 20 insertions(+), 8 deletions(-) > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > index 6855b94bbf..99666383b2 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -107,6 +107,22 @@ extern int daemon(int, int); > #include "glib-compat.h" > #include "qemu/typedefs.h" > > +/* > + * We have a lot of unaudited code that may fail in strange ways, or > + * even be a security risk during migration, if you disable assertions > + * at compile-time. You may comment out these safety checks if you > + * absolutely want to disable assertion overhead, but it is not > + * supported upstream so the risk is all yours. Meanwhile, please > + * submit patches to remove any side-effects inside an assertion, or > + * fixing error handling that should use Error instead of assert. > + */ > +#ifdef NDEBUG > +#error building with NDEBUG is not supported > +#endif > +#ifdef G_DISABLE_ASSERT > +#error building with G_DISABLE_ASSERT is not supported > +#endif > + > #ifndef O_LARGEFILE > #define O_LARGEFILE 0 > #endif > diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c > index 765ab53c34..9962286bd6 100644 > --- a/hw/scsi/mptsas.c > +++ b/hw/scsi/mptsas.c > @@ -1236,11 +1236,9 @@ static void *mptsas_load_request(QEMUFile *f, > SCSIRequest *sreq) > n = qemu_get_be32(f); > /* TODO: add a way for SCSIBusInfo's load_request to fail, > * and fail migration instead of asserting here. > - * When we do, we might be able to re-enable NDEBUG below. > + * This is just one thing (there are probably more) that must be > + * fixed before we can allow NDEBUG compilation. > */ > -#ifdef NDEBUG > -#error building with NDEBUG is not supported > -#endif > assert(n >= 0); > > pci_dma_sglist_init(&req->qsg, pci, n); > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 464947f76d..3129d25c00 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -1025,11 +1025,9 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, > QEMUFile *f, size_t sz) > > /* TODO: teach all callers that this can fail, and return failure instead > * of asserting here. > - * When we do, we might be able to re-enable NDEBUG below. > + * This is just one thing (there are probably more) that must be > + * fixed before we can allow NDEBUG compilation. > */ > -#ifdef NDEBUG > -#error building with NDEBUG is not supported > -#endif > assert(ARRAY_SIZE(data.in_addr) >= data.in_num); > assert(ARRAY_SIZE(data.out_addr) >= data.out_num); >
Queued, thanks. Paolo
