Closed Bug 1553037 Opened 4 months ago Closed 4 months ago

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

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set

Tracking

(firefox69 fixed)

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: gerald, Assigned: andi)

Details

Attachments

(3 files)

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".)

@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.

Flags: needinfo?(gsquelart)
Assignee: nobody → bpostelnicu

Running static analysis on Windows.

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

Flags: needinfo?(gsquelart)

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

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

@gerald: please do the following:

  • delete the obj directory
  • use an empty .mozconfig

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

Flags: needinfo?(gsquelart)

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.

Flags: needinfo?(gsquelart)
Summary: [Automated review] Strange false positive: Dereference of null pointer [clang-analyzer-core.NullDereference] → [Automated review] Should suppress intentional positive: Dereference of null pointer [clang-analyzer-core.NullDereference] in MOZ_REALLY_CRASH

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

Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dafb8769ab48
clang-based disable static-analysis in `MOZ_REALLY_CRASH`. r=froydnj
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.