New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Silence false positive clang-tidy warning #727

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@kkaefer

kkaefer commented Sep 1, 2016

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

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 1, 2016

Coverage Status

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

coveralls commented Sep 1, 2016

Coverage Status

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

@miloyip

This comment has been minimized.

Show comment
Hide comment
@miloyip

miloyip Sep 1, 2016

Collaborator

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

Collaborator

miloyip commented Sep 1, 2016

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

@miloyip

This comment has been minimized.

Show comment
Hide comment
@miloyip

miloyip Jun 21, 2018

Collaborator

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

Collaborator

miloyip commented Jun 21, 2018

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

@mattgodbolt

This comment has been minimized.

Show comment
Hide comment
@mattgodbolt

mattgodbolt Oct 1, 2018

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.

mattgodbolt commented Oct 1, 2018

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

This comment has been minimized.

Show comment
Hide comment
@pah

pah Oct 2, 2018

Contributor

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.

Contributor

pah commented Oct 2, 2018

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.

Avoid pointer arithmetic on null pointer to remove undefined behavior
The existing checks triggered undefined behavior when the stack was empty (null pointer). This change avoid this:
* 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
@kkaefer

This comment has been minimized.

Show comment
Hide comment
@kkaefer

kkaefer Oct 5, 2018

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.

kkaefer commented Oct 5, 2018

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

This comment has been minimized.

Show comment
Hide comment
@tencent-adm

tencent-adm Oct 5, 2018

CLA assistant check
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.

tencent-adm commented Oct 5, 2018

CLA assistant check
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

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 5, 2018

Coverage Status

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

coveralls commented Oct 5, 2018

Coverage Status

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment