Silence false positive clang-tidy warning

[kkaefer]

The existing check can't be understood by clang-analyze/clang-tidy and reports a null pointer dereference down the line and produces these warnings:

include/rapidjson/internal/itoa.h:52:19: warning: Dereference of null pointer [clang-analyzer-core.NullDereference]
        *buffer++ = cDigitsLut[d2 + 1];
                  ^
tidy.cpp:7:5: note: Calling 'Writer::Int'
    writer.Int(1);
    ^
include/rapidjson/writer.h:174:72: note: Calling 'Writer::WriteInt'
    bool Int(int i)             { Prefix(kNumberType); return EndValue(WriteInt(i)); }
                                                                       ^
include/rapidjson/writer.h:480:23: note: Calling 'i32toa'
    const char* end = internal::i32toa(i, buffer);
                      ^
include/rapidjson/internal/itoa.h:115:5: note: Taking false branch
    if (value < 0) {
    ^
include/rapidjson/internal/itoa.h:120:12: note: Calling 'u32toa'
    return u32toa(u, buffer);
           ^
include/rapidjson/internal/itoa.h:42:5: note: Taking true branch
    if (value < 10000) {
    ^
include/rapidjson/internal/itoa.h:46:9: note: Taking false branch
        if (value >= 1000)
        ^
include/rapidjson/internal/itoa.h:48:9: note: Taking false branch
        if (value >= 100)
        ^
include/rapidjson/internal/itoa.h:50:9: note: Taking false branch
        if (value >= 10)
        ^
include/rapidjson/internal/itoa.h:52:19: note: Dereference of null pointer
        *buffer++ = cDigitsLut[d2 + 1];
                  ^

(similar for other types of writers)

The sample program I used is

#include <rapidjson/writer.h>

int main(int, char*[]) {
    using namespace rapidjson;
    StringBuffer sb;
    Writer<StringBuffer> writer(sb);
    writer.Int(1);
    return 0;
}

It seems that clang-tidy is not sure that an allocation ever happens.

[coveralls]

Coverage remained the same at 99.937% when pulling 6c9a0f4 on mapbox:silence-dereference-null-pointer into cdb3454 on miloyip:master.

[miloyip]

Is there other ways to suppress the false alarm?
I do not want to please those checkers with potential performance overheads.

[miloyip]

Can it be done by adding a RAPIDJSON_ASSERT() instead?

[mattgodbolt]

I'm seeing this too. Adding // NOLINT on the include/rapidjson/internal/itoa.h:52 line is effective, if somewhat unfortunate. I don't think a RAPIDJSON_ASSERT() could help here: if it compiles out on release, then our clang-tidy checks will fail in release mode, and if it doesn't compile away it will pessimize performance.

[pah]

I don't think a RAPIDJSON_ASSERT() could help here: if it compiles out on release, then our clang-tidy checks will fail in release mode, and if it doesn't compile away it will pessimize performance.

Why would you want to run clang-tidy with NDEBUG defined? I assume there a tons of other places, where this would then trigger such false positives.

[kkaefer]

Looking at this again, I believe that this is a valid bug, not merely a bogus clang-tidy warning: When stackTop_ is a null pointer ("dangling pointer"), arithmetic on it is undefined (see https://kristerw.blogspot.com/2016/03/c-pointers-are-not-hardware-pointers.html for more background). The check relies on this pointer arithmetic working like integer arithmetic. That's true in most cases and for most compilers, but not valid according to the specification. This pull request changes it to check for a valid stackTop_ pointer first before attempting to do pointer arithmetic.

This may still trigger undefined behavior because we're producing an out of bound value when adding to stackTop_ though, so a better check would be:

if (RAPIDJSON_UNLIKELY(sizeof(T) * count > stackEnd_ - stackTop_)) {

I believe this avoid UB:

• If stackTop_ and stackEnd_ are null, it results in a ptrdiff_t of 0
• If stackTop_ and stackEnd_ are valid pointers, they produce a ptrdiff_t with the remaining size on the stack.

I pushed an update to this branch with the new fix.

[tencent-adm]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[coveralls]

Coverage increased (+0.001%) to 99.922% when pulling 16872af on mapbox:silence-dereference-null-pointer into 663f076 on Tencent:master.

[rigtorp]

I can confirm this fix silences the warning. Would be great if it could be merged.