Bug 188741 – [JSC] Suppress UBSan warnings for KeywordLookup

• Component: New Bugs
• Status: NEW

[Yusuke Suzuki]

[JSC] Suppress UBSan warnings for KeywordLookup

[Yusuke Suzuki]

Created attachment 347489 [details]
Patch

[Fujii Hironori]

Can you use WTF::unalignedLoad instead of reinterpret_cast in this case?

There is a code path for CPU(NEEDS_ALIGNED_ACCESS). How about the idea to use the code path if UBSan is enabled?

[Fujii Hironori]

Don, 

IIUC, we (SIE) want to remove undefined behaviors as much as possible for CFI. Right?

Control Flow Integrity
https://clang.llvm.org/docs/ControlFlowIntegrity.html

[Don Olmstead]

(In reply to Fujii Hironori from comment #3)
> Don, 
> 
> IIUC, we (SIE) want to remove undefined behaviors as much as possible for
> CFI. Right?
> 
> Control Flow Integrity
> https://clang.llvm.org/docs/ControlFlowIntegrity.html

For CFI the unaligned loads should not be a problem. Patch as is looks fine I'm just interested in knowing if something like the WTF functions mentioned would be of use here just for reuse purposes.

[Filip Pizlo]

Comment on attachment 347489 [details]
Patch

I’m not sure we should be splatting these blacklist things - it makes the code uglier.

[Don Olmstead]

(In reply to Filip Pizlo from comment #5)
> Comment on attachment 347489 [details]
> Patch
> 
> I’m not sure we should be splatting these blacklist things - it makes the
> code uglier.

We can use a blacklist file or annotate the code. The benefit from having it in code is that you can see right away that the code is relying on undefined behavior.

I don't have a strong opinion either way.

[Michael Catanzaro]

(In reply to Filip Pizlo from comment #5)
> Comment on attachment 347489 [details]
> Patch
> 
> I’m not sure we should be splatting these blacklist things - it makes the
> code uglier.

This looks like really low-cost to me. WebKit already has SUPPRESS_ASAN because we recognize asan is an extremely valuable tool for finding security bugs, and using SUPPRESS_ASAN in a couple odd places is less-invasive than rewriting those places to not trigger asan. (If we don't rewrite the code, and don't use SUPPRESS_ASAN, then we simply can't use asan.)

Now, ubsan is less-valuable than asan, but that's only because asan is really really good. Making a couple small tweaks to the code like this to please ubsan seems valuable to me, since it will make it much easier to run WebKit under ubsan. This seems much more valuable to me than a blacklist file, since that would require extra work to configure and would reduce the ease of running WebKit under ubsan.

[Filip Pizlo]

(In reply to Michael Catanzaro from comment #7)
> (In reply to Filip Pizlo from comment #5)
> > Comment on attachment 347489 [details]
> > Patch
> > 
> > I’m not sure we should be splatting these blacklist things - it makes the
> > code uglier.
> 
> This looks like really low-cost to me. WebKit already has SUPPRESS_ASAN
> because we recognize asan is an extremely valuable tool for finding security
> bugs, and using SUPPRESS_ASAN in a couple odd places is less-invasive than
> rewriting those places to not trigger asan. (If we don't rewrite the code,
> and don't use SUPPRESS_ASAN, then we simply can't use asan.)

I don’t mind these annotations for asan. I was only talking about ubsan. 

> 
> Now, ubsan is less-valuable than asan, but that's only because asan is
> really really good. Making a couple small tweaks to the code like this to
> please ubsan seems valuable to me, since it will make it much easier to run
> WebKit under ubsan. This seems much more valuable to me than a blacklist
> file, since that would require extra work to configure and would reduce the
> ease of running WebKit under ubsan.

Ubsan is such a silly concept. The case of reporting bad shift amounts as errors is particularly silly: the only fix for that is to mask the shift amount. If that’s the only fix then we should ask for something that just applies those masks for us everywhere. 

The same argument can be made for most other kinds of UB.

[Saagar Jha]

The code exhibiting undefined behavior that this bug mentions has been rewritten to be standards-compliant in https://bugs.webkit.org/show_bug.cgi?id=199650, so I don't think we need that part of the changeset anymore. I think that adding annotations is still useful, however: there are lot of places in WebKit that remain noncompliant, and an annotation like this would be useful to mark all the places we haven't gotten around to fixing yet as well as ensuring that new code remains free of undefined behavior.

[Yusuke Suzuki]

Comment on attachment 347489 [details]
Patch

For now, I'm not planning to do this.