search

bus1 / dbus-broker

Linux D-Bus Message Broker
https://github.com/bus1/dbus-broker/wiki
Apache License 2.0
675 stars 78 forks source link

Coverity fails to parse code of dbus-broker #237

Closed kdudka closed 3 years ago

kdudka commented 4 years ago

I tried to statically analyze dbus-broker with Coverity Analysis 2020.06 but 32% of compilation units failed to parse. The most frequent parsing errors were these:

"../src/dbus/address.h", line 26: error #28: expression must have a constant
          value
          char buffer[ADDRESS_ID_STRING_MAX + 1];
                      ^
"../src/dbus/message.h", line 91: error #28: expression must have a constant
          value
          alignas(8) uint8_t patch[MESSAGE_PATCH_MAX];
                                   ^

It seems to be caused by the C_DECIMAL_MAX macro definition. Is this a known issue? If yes, is there any workaround for this?

dvdhrm commented 4 years ago

I assume it trips over the macro-logic in c-stdaux. You can try analysing just the c-stdaux code (https://github.com/c-util/c-stdaux) and see which of the macros cause the problem. If it is C_EXPR_ASSERT() you could do something like the following to replace the macro with a direct evaluation:

#define C_EXPR_ASSERT(_expr, _assertion, _message) (_expr)

Both gcc and clang parse the code just fine. Not sure why coverity fails. Do you happen to know what they use internally? Do they use the clang libraries?

kdudka commented 4 years ago

Thank you for having a look at this! I will give it a try when I am back from vacation. Good news is that the compilation success rate was already increased to 79% after applying this patch on system header files: https://github.com/systemd/systemd/commit/4191b328

kdudka commented 4 years ago

Compilation of c-stdaux tests does not trigger the parsing problem in Coverity. The following code does:

#include <c-stdaux.h>

struct str {
    char buf[C_DECIMAL_MAX(int)];                                                             
};

The proposed redefinition of C_EXPR_ASSERT fixes it.

dvdhrm commented 4 years ago

The C_DECIMAL_MAX macro uses the C_EXPR_ASSERT macro, which itself uses _Static_assert() as well as __builtin_choose_expr(). Coverity somehow lacks support for one of those (a search suggests it is the latter).

I don't know of any alternative to make this macro work. We use this quite a lot to allow the same functions to be used in constant-expressions as well as normal statements.

kdudka commented 4 years ago

Cannot we fix it like this?

#ifdef __COVERITY__
#   define C_EXPR_ASSERT(_expr, _assertion, _message) (_expr)
#else
#   define C_EXPR_ASSERT(_expr, _assertion, _message) ...
#endif
dvdhrm commented 4 years ago

Cannot we fix it like this?

#ifdef __COVERITY__
#   define C_EXPR_ASSERT(_expr, _assertion, _message) (_expr)
#else
#   define C_EXPR_ASSERT(_expr, _assertion, _message) ...
#endif

Well, yeah, we can add workarounds. Though I would prefer if there was a bug-report against Coverity so we have an idea for how long we have to carry this workaround.

Can you elaborate why this is needed? Do you intend to run coverity regularly?

kdudka commented 4 years ago

Absolutely. We (Red Hat) do full scan/review of all RPM packages on each major release of RHEL and differential scan/review on each minor update of RHEL. If you are not aware of that, it either means that your packages are sane, or that their bugs are just well hidden behind compatibility issues like this ;-)

dvdhrm commented 3 years ago

I now pushed the Coverity-guard to c-stdaux and pulled it into dbus-broker. If there are other issues, let me know.

dvdhrm commented 3 years ago

Thanks for the report!

kdudka commented 3 years ago

Nice. Thank you for putting the workaround in place!