Skip to content
  • Eric Blake's avatar
    262a69f4
    osdep.h: Prohibit disabling assert() in supported builds · 262a69f4
    Eric Blake authored
    
    
    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: default avatarEric Blake <eblake@redhat.com>
    Reviewed-by: default avatarPhilippe Mathieu-Daudé <f4bug@amsat.org>
    Reviewed-by: default avatarThomas Huth <thuth@redhat.com>
    
    Message-Id: <20170911211320.25385-1-eblake@redhat.com>
    Signed-off-by: default avatarPaolo Bonzini <pbonzini@redhat.com>
    262a69f4
    osdep.h: Prohibit disabling assert() in supported builds
    Eric Blake authored
    
    
    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: default avatarEric Blake <eblake@redhat.com>
    Reviewed-by: default avatarPhilippe Mathieu-Daudé <f4bug@amsat.org>
    Reviewed-by: default avatarThomas Huth <thuth@redhat.com>
    
    Message-Id: <20170911211320.25385-1-eblake@redhat.com>
    Signed-off-by: default avatarPaolo Bonzini <pbonzini@redhat.com>
Loading