1553037 - [Automated review] Should suppress intentional positive: Dereference of null pointer [clang-analyzer-core.NullDereference] in MOZ_REALLY_CRASH

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Phabricator URL: https://phabricator.services.mozilla.com/D31930

This is a very puzzling warning:
All this patch did was add a namespace around functions.
A previous patch (in the same set) with the same code but no namespace didn't raise any alarms.

./mach static-analysis check mozglue/tests/TestMozProfiler.cpp doesn't reproduce the warning locally for me.
It would be nice if we could get more details about the issue directly in phabricator.
(And also if we could say "don't bother me with this anymore".)

Comment 1

[Andi [:andi]]

@gerald can you please paste here what's the output of running static-analysis on mozglue/tests/TestMozProfiler.cpp. I've seen in a related issue that you are running windows? Maybe it's an inconsistency since we are running our static-analysis on linux64.
Also do by any chance you build with: ac_add_options --enable-debug in mozconfig?
I'm asking this because looking at the logs we can clearly see that the issue appears in the implementation of MOZ_REALLY_CRASH but in case DEBUG is set then we don't actually call MOZ_CRASH that invokes MOZ_REALLY_CRASH, see MOZ_CRASH definition.
But this somehow shows an issue with our analysis and that it would be better to use ac_add_options --enable-debug for the analysis.

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: TestMozProfiler-check-win.txt

Running static analysis on Windows.

Yes, I have --enable-debug, also --enable-optimize="-O1" if that's relevant.

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: TestMozProfiler-check-linux.txt

Static analysis on 64-bit Linux, also not reproducing the Phabricator errors.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Tried again on WIndows with --enable-optimize and no debug, got the same result.

Comment 5

[Andi [:andi]]

@gerald: please do the following:

• delete the obj directory
• use an empty .mozconfig

Run again ./mach static-analysis check mozglue/tests/TestMozProfiler.cpp

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

On windows, with no obj and no mozconfig, that fixed the errors, but still no warnings... But I've just realized that this code is disabled on Windows! So please ignore my complaint about not being able to reproduce on Windows, my mistake.

However, good news everyone, I tried with no obj nor mozconfig on Linux, and could reproduce the issue. Looks like MOZ_REALLY_CRASH's crashing code is flagged:

[...]
obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:448:7: note: expanded from macro 'MOZ_ASSERT_HELPER1'
      MOZ_REALLY_CRASH(__LINE__);                              \
      ^
obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:247:48: note: expanded from macro 'MOZ_REALLY_CRASH'
        *((volatile int*)MOZ_CRASH_WRITE_ADDR) = line; \
                                               ^

It would be really useful to get these details in Phabricator. But then it would also be quite annoying to get pages of these, especially for false-positives... Any way Phabricator could link to an external file with the full output? (If yes, would you mind opening a separate bug? TIA)

And it's still strange that that was not picked up in previous patches in Phabricator. Trying locally, I did get the warnings there. Does Phabricator run the check once for the stack, and then assigns warnings to the last patch touching a line with a warning?

Anyway, back to this bug here:
The warning is actually correct, we do dereference a nullptr, but it is intentional to force a crash! (I've updated the bug title to reflect that this is not a false positive.)

It should be possible to somehow inform clang-tidy that this line should be ignored.
Reading https://clang.llvm.org/extra/clang-tidy/ I see:

clang-tidy has a generic mechanism to suppress diagnostics using NOLINT or NOLINTNEXTLINE comments.

How about we add this there, to avoid future warnings? Please let me know if you agree; happy to do it if you'd like me to.

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(And in other good news, I was able to reproduce the warning in Windows, by force-enabling the code. So all is well 🙂)

Comment 8

[Andi [:andi]]

Attachment: Bug 1553037 - clang-based disable static-analysis in `MOZ_REALLY_CRASH`. r=froydnj

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dafb8769ab48
clang-based disable static-analysis in `MOZ_REALLY_CRASH`. r=froydnj

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/dafb8769ab48