[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

I will still need to see where else `EmitCheckedInBoundsGEP()` should be used, but i'm wondering
if it should be best done in follow-ups, since this already catches the interesting bits..

In D67122#1659721 <https://reviews.llvm.org/D67122#1659721>, @vsk wrote:

> Still think this looks good. Have you tried running this on the llvm test suite, or some other interesting corpus? Would be curious to see any pre/post patch numbers.


There were 1.5 occurrences in test-suite.
I'd prefer not to try running it over llvm stage-2/3 (i can, but it's gonna be slow..)

Though i guess you were talking about *performance* numbers?

I'm not good with using test-suite for perf, so i'm yet again going to use my own bench.
F9934220: rawspeed-pointer-overflow-0-baseline.json <https://reviews.llvm.org/F9934220> F9934221: rawspeed-pointer-overflow-1-old.json <https://reviews.llvm.org/F9934221> F9934222: rawspeed-pointer-overflow-2-new.json <https://reviews.llvm.org/F9934222>

TLDR: (all measurements done with llvm ToT, the sanitizer never fired.)

- no sanitization vs. existing check: average `+20.75%` slowdown
- existing check vs. check after this patch: average `+26.36%` slowdown
- no sanitization vs. this patch: average `+52.58%` slowdown

I suspect some of this might be recoverable, but i don't know yet.

@vsk let me know if that answered your questions. thanks!

In D67122#1659896 <https://reviews.llvm.org/D67122#1659896>, @lebedev.ri wrote:

> In D67122#1659882 <https://reviews.llvm.org/D67122#1659882>, @rupprecht wrote:
>
> > > Still think this looks good. Have you tried running this on the llvm test suite, or some other interesting corpus? Would be curious to see any pre/post patch numbers.
> >
> > I finally had time this morning to patch this in and give it a shot. (Sorry for the delay... it's been a real busy week :( )
>
>
>
>
> > First, starting off with the good news: I reverted all the fixes I made, and now all the tests fail when running w/ ubsan. Yay!
> >  The error we see in each case is `UndefinedBehaviorSanitizer: nullptr-with-nonzero-offset` with the logs containing `runtime error: applying non-zero offset <non-zero> to null pointer`. Which catches the two places where we were adding some non-zero offset to nullptr,
>
> That is good :)
>
> > but doesn't seem to catch the nullptr-after-nonzero-offset case in https://github.com/google/filament/pull/1566 -- instead, it fails later, when the pointer with a value of nullptr is incremented. (Or... maybe this is actually a separate bug. Hmm. Needs some more testing...)
>
> Well yeah, there are quite a few cases in clang codegen where we create `gep inbounds` without sanitization, so it may or may not be one of those cases.
>
> > At any rate, I have some more tests to run to get some idea of what % of code this would flag as being bad.


@rupprecht

So i have tried to reduce https://github.com/google/filament/pull/1566/files
`CommandBase` by hand, and arrived at: https://godbolt.org/z/O7svxp

And it is clearly being sanitized, and you wouldn't even get to call `execute()`
with `this = nullptr`, a check for that was inserted too.

If you revert whatever fixes that were applied, fix the code
about which sanitizer complaints, does that fix miscompilation?

But TLDR, either the fix in https://github.com/google/filament/pull/1566
is incorrect and the actually-bad code is elsewhere,
or you have some other unsanitized UB elsewhere. Could be both :S


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67122/new/

https://reviews.llvm.org/D67122