1469410 - UBSan false positive at tools/profiler/lul/LulMain.cpp:910:57

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P3)

Description

[Tyson Smith [:tsmith]]

STR:
1) Add additional options to CFLAGS & CXXFLAG in the latest ASan mozconfig "-fsanitize=bool,bounds,vla-bound -fno-sanitize-recover=bool,bounds,vla-bound"
2) Build
3) Run gtests

Full logs can be found here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a06cd005cfa6eb08100a6b04dd4a6165658c505

[task 2018-06-18T20:54:29.876Z] 20:54:29     INFO -  TEST-START | GeckoProfiler.FeaturesAndParams
[task 2018-06-18T20:54:35.829Z] 20:54:35     INFO -  /builds/worker/workspace/build/src/tools/profiler/lul/LulMain.cpp:910:57: runtime error: index 140730742006808 out of bounds for type 'uint8_t const[163840]'
[task 2018-06-18T20:54:35.836Z] 20:54:35     INFO -  SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /builds/worker/workspace/build/src/tools/profiler/lul/LulMain.cpp:910:57 in
[task 2018-06-18T20:54:36.062Z] 20:54:36    ERROR -  gtest TEST-UNEXPECTED-FAIL | gtest | test failed with return code 1

Comment 1

[Tyson Smith [:tsmith]]

Attachment: repro_patch.diff

This is the patch that was applied before running the gtest to trigger the failure

Comment 2

[Julian Seward [:jseward]]

I can't reproduce this locally, with an asan build with the recommended flags.

There's something strange about it, too.  What it is complaining about is an 
access into the stack-snapshot array, which has size 163840 bytes.  It says
that the index into the array is 140730742006808, which is way out of bounds.
But the access is guarded by a bounds check (src/tools/profiler/lul/LulMain.cpp:898),
which I believe to be correct, and the two places where the limit (aStackImg->mLen)
is set also look correct.  So I don't see how this can have happened.

Comment 3

[Julian Seward [:jseward]]

Reproduced.  But with GeckoProfiler.GetBacktrace, not GeckoProfiler.FeaturesAndParams.

Comment 4

[Julian Seward [:jseward]]

This appears to be caused by the computation

  return TaggedUWord(*(uintptr_t*)(aStackImg->mContents + aAddr.Value()
                                   - aStackImg->mStartAvma));

and in particular this

  aStackImg->mContents + ...

aStackImg->mContents has type 'uint8_t[N_STACK_BYTES]'.  I suspect that
UBSan is complaining about using it to form an address without explicitly
decaying into a non-array type.  Although, given that the final computed
address falls within the array, I'm not really clear what the objection
is.  But in any case, an explicit cast seems to fix the problem.

Comment 5

[Julian Seward [:jseward]]

Attachment: bug1469410-decay-mContents-1.diff

Nathan, what says your C++ expertise?  In the patch I've tried to
be as explicit as I can that "I want to form an address and then
I want to cast it to a uintptr_t, and only then do I want to add/sub
a couple of offsets."

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8986419 [details] [diff] [review]
bug1469410-decay-mContents-1.diff

Review of attachment 8986419 [details] [diff] [review]:
-----------------------------------------------------------------

I think the suggestion below is better, but whatever, this is gnarly code no matter what.

::: tools/profiler/lul/LulMain.cpp
@@ +908,5 @@
>    }
>  
> +  return TaggedUWord(*(uintptr_t*)( (uintptr_t)(&aStackImg->mContents[0])
> +                                    + aAddr.Value()
> +                                    - aStackImg->mStartAvma ));

This is a lot of...stuff.  Can't we just say:

*(uintptr_t*)&aStackImg->mContents[aAddr.Value() - aStackImg->mStartAvma];

to make ubsan happy?  (memcpy'ing it to a separate uintptr_t, rather than C-style casting, would also be nice, I suppose.)

Comment 7

[Andrew McCreight [:mccr8]]

I'm going to mark this as sec-audit as this sounds like undefined behavior that is not currently being exploited by compilers.

Comment 8

[Julian Seward [:jseward]]

Looks OK on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c92e7a465630226008f0315101b4e2af135c38ee
I assume that the big bunch of windows10-64-ccov debug and macosx64-ccov
debug failures (timeouts) are unrelated, given that this patch doesn't touch
any windows code.

I agree with mccr8's assessment in comment 7.  Requesting sec-review since
I am unsure how to proceed.  AFAICS there is no actual bounds violation,
otherwise this would have crashed spectacularly long ago, when the Gecko
profiler is running, on Linux and targets.  OTOH it does involve undefined
behaviour in C++, and it does affect everything back to m-release and probably
some esrs too, since this is in a part of the Gecko profiler that has been
stable for at least a year.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Julian Seward [:jseward] from comment #8)
> I agree with mccr8's assessment in comment 7.  Requesting sec-review since
> I am unsure how to proceed.  AFAICS there is no actual bounds violation,
> otherwise this would have crashed spectacularly long ago, when the Gecko
> profiler is running, on Linux and targets.  OTOH it does involve undefined
> behaviour in C++, and it does affect everything back to m-release and
> probably
> some esrs too, since this is in a part of the Gecko profiler that has been
> stable for at least a year.

sec-audit means you don't have to ask for sec-review.  And I think this is a false positive from ubsan anyway, given that we already do bounds checking; we're just rewriting the code into a form that doesn't (wrongly) trigger ubsan's detector.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b04c3943f7b2
UBSan false positive at tools/profiler/lul/LulMain.cpp:910:57.  r=froydnj.

Comment 11

[Arthur Iakab [arthur_iakab]]

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