Closed Bug 1469410 Opened Last year Closed Last year

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

Categories

(Core :: Gecko Profiler, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: tsmith, Assigned: jseward)

References

Details

(Keywords: csectype-bounds, csectype-undefined, sec-audit, Whiteboard: [adv-main62-])

Attachments

(2 files)

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
Attached file repro_patch.diff
This is the patch that was applied before running the gtest to trigger the failure
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.
Reproduced.  But with GeckoProfiler.GetBacktrace, not GeckoProfiler.FeaturesAndParams.
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.
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."
Attachment #8986419 - Flags: review?(nfroyd)
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.)
Attachment #8986419 - Flags: review?(nfroyd) → review+
Assignee: nobody → jseward
Priority: -- → P3
Blocks: 1469910
No longer blocks: ubsan
I'm going to mark this as sec-audit as this sounds like undefined behavior that is not currently being exploited by compilers.
Keywords: sec-audit
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.
(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.
Summary: UBSan: index out of bounds in tools/profiler/lul/LulMain.cpp:910:57 → UBSan false positive at tools/profiler/lul/LulMain.cpp:910:57
Group: core-security
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b04c3943f7b2
UBSan false positive at tools/profiler/lul/LulMain.cpp:910:57.  r=froydnj.
https://hg.mozilla.org/mozilla-central/rev/b04c3943f7b2
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Whiteboard: [adv-main62+]
Whiteboard: [adv-main62+] → [adv-main62-]
You need to log in before you can comment on or make changes to this bug.