1285057 - Add ability to blacklist functions from UBSan and use it to suppress GC's generic calls

Status: RESOLVED WORKSFORME

(Core :: JavaScript: GC, defect, P3)

Description

[Terrence Cole [:terrence]]

Attachment: ubsan-blacklist-generic-indirection-v0.diff

In order to implement Rooted<struct> and WeakCache, we expect to be able to call a function with the wrong type. This is undefined, obviously, but relatively safe as long as everything is a pointer. When I tried to architect things on virtual before, I ran into heinous dependency cycles. I think for now at least we want to suppress these sites instead of reworking our rooting architecture.

Comment 1

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fe87b0c8181

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8768593 [details] [diff] [review]
ubsan-blacklist-generic-indirection-v0.diff

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

::: mfbt/Attributes.h
@@ +292,5 @@
> + */
> +#if defined(__clang__)
> +#  define MOZ_UBSAN_BLACKLIST MOZ_NEVER_INLINE __attribute__((no_sanitize("function")))
> +#else
> +#  define MOZ_UBSAN_BLACKLIST /* nothing */

I know it's messy, but...

First, since this is blacklisting only mistyped function pointer calls, this should be MOZ_UBSAN_BLACKLIST_FUNCTION (to match the attribute name, though it really makes it sound like you're blacklisting an entire function. Which you are, but that has nothing to do with the "function" part.)

Second, I'd like this to work in both clang and gcc. Annoyingly, they don't seem to support the same syntax (or functionality, even). So I think this'll have to be

#if defined(__clang__)
# define MOZ_UBSAN_BLACKLIST_FUNCTION MOZ_NEVER_INLINE __attribute__((no_sanitize("function")))
#elif defined(__GNUC__)
# define MOZ_UBSAN_BLACKLIST_FUNCTION MOZ_NEVER_INLINE __attribute__((no_sanitize_undefined))
#else
# define MOZ_UBSAN_BLACKLIST_FUNCTION /* nothing */
#endif

Third, and don't do anything about this, but calling it a "blacklist" seems like a double-negative. You're blacklisting it from the list of things to check for problems. I would've called it a whitelist, But there are other flags that use the term blacklist for this, so this is just me complaining, not a request for a change. ;-)

Comment 3

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d71694334dc1

Comment 4

[Terrence Cole [:terrence]]

Hurm, in all the GCC builds I'm getting:

obj-spider/dist/include/js/SweepingAPI.h:60:45: error: 'no_sanitize_undefined' attribute directive ignored [-Werror=attributes]

Comment 5

[Terrence Cole [:terrence]]

GCC 4.8 is too old for UBSan, so I guess we'll need to just support clang for now.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d85334f696eb
Blacklist UBSan detection of the GC's generic interfaces; r=sfink

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Jan Varga [:janv]]

The patch doesn't compile for me. Mac 10.10.5, Xcode 7.2.1

Comment 10

[Terrence Cole [:terrence]]

What are the failures?

Comment 11

[Jan Varga [:janv]]

/usr/local/bin/ccache /usr/bin/clang++ -std=gnu++11 -o Unified_cpp_dom_presentation0.o -c  -fvisibility=hidden -fvisibility-inlines-hidden -DDEBUG=1 -DTRACING=1 -DOS_POSIX=1 -DOS_MACOSX=1 -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/Users/varga/Sources/Mozilla/dom/presentation -I/Users/varga/Sources/Mozilla/obj-ff-dbg/dom/presentation -I/Users/varga/Sources/Mozilla/dom/base -I/Users/varga/Sources/Mozilla/obj-ff-dbg/ipc/ipdl/_ipdlheaders -I/Users/varga/Sources/Mozilla/ipc/chromium/src -I/Users/varga/Sources/Mozilla/ipc/glue -I/Users/varga/Sources/Mozilla/obj-ff-dbg/dist/include  -I/Users/varga/Sources/Mozilla/obj-ff-dbg/dist/include/nspr -I/Users/varga/Sources/Mozilla/obj-ff-dbg/dist/include/nss       -fPIC  -DMOZILLA_CLIENT -include /Users/varga/Sources/Mozilla/obj-ff-dbg/mozilla-config.h -MD -MP -MF .deps/Unified_cpp_dom_presentation0.o.pp -Qunused-arguments  -Qunused-arguments -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wloop-analysis -Wc++11-compat-pedantic -Wc++14-compat -Wc++14-compat-pedantic -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wthread-safety -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-exceptions -fno-strict-aliasing -stdlib=libc++ -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe  -g -O3 -fno-omit-frame-pointer  -Werror   /Users/varga/Sources/Mozilla/obj-ff-dbg/dom/presentation/Unified_cpp_dom_presentation0.cpp
In file included from /Users/varga/Sources/Mozilla/obj-ff-dbg/dom/presentation/Unified_cpp_dom_presentation0.cpp:2:
In file included from /Users/varga/Sources/Mozilla/dom/presentation/AvailabilityCollection.cpp:10:
In file included from /Users/varga/Sources/Mozilla/dom/presentation/PresentationAvailability.h:10:
In file included from /Users/varga/Sources/Mozilla/obj-ff-dbg/dist/include/mozilla/DOMEventTargetHelper.h:12:
In file included from /Users/varga/Sources/Mozilla/obj-ff-dbg/dist/include/nsCycleCollectionParticipant.h:13:
/Users/varga/Sources/Mozilla/obj-ff-dbg/dist/include/js/RootingAPI.h:649:5: error: unknown attribute 'no_sanitize' ignored [-Werror,-Wunknown-attributes]
    MOZ_UBSAN_BLACKLIST_FUNCTION static void TraceWrapped(JSTracer* trc, T* thingp,
    ^
/Users/varga/Sources/Mozilla/obj-ff-dbg/dist/include/mozilla/Attributes.h:219:71: note: expanded from macro 'MOZ_UBSAN_BLACKLIST_FUNCTION'
# define MOZ_UBSAN_BLACKLIST_FUNCTION MOZ_NEVER_INLINE __attribute__((no_sanitize("function")))
                                                                      ^
1 error generated.
make[1]: *** [Unified_cpp_dom_presentation0.o] Error 1
make: *** [default] Error 2

Comment 12

[Terrence Cole [:terrence]]

The no_sanitize flag was added in 3.8 and is not present in 3.6. In XCode 7.2.1 on OSX 10.10, the LLVM version number is "Apple LLVM version 7.0.2 (clang-700.1.81)", which, sadly, is not particularly useful. Presumably it's some version of 3.6 that does not include the new flag.

Looking at "Mac OS X Build Prerequisites" on MDN, it's really not clear what our minimum compiler version is, if we even have one. I guess since xcode makes it impossible to even know what version of clang you're running, much less control what version you're running, there's little point specifying.

So I guess maybe we should fix the wiki? Or I can backout, figure out how to feature gate this feature gate? I'll ask around to see if anyone actually knows what our minimum version is here and why.

Comment 13

[Terrence Cole [:terrence]]

I guess we are still on 3.6, so here is a backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/16daeb9b4e17

Comment 14

[Carsten Book [:Tomcat]]

reopen because of backout as mentioned in #13

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

You just need my patch in bug 1305345.

Comment 16

[:dmajor]

This bug has been stalled for several years. Given how much the JS engine changes in that time, is the problem with sweep() still present?

In the time since we've also gained more specific ubsan-disabling annotations, such as https://searchfox.org/mozilla-central/rev/ebe492edacc75bb122a2b380e4cafcca3470864c/mfbt/Attributes.h#295.

Comment 17

[Jon Coppeard (:jonco)]

(In reply to :dmajor from comment #16)
The problem is still present and has just been independently discovered in bug 1587173. I couldn't see a macro to disable checked for this particular type of UB. I don't know how serious this kind of UB is and whether it would be OK to blacklist it or whether we should find a way to fix it. From the description it sounds like the obvious fix is not viable.

Comment 18

[:dmajor]

I think I must have mis-read the old patch here. On closer look it should be fine to re-land the Attributes.h piece and use it as needed. Our minimum compiler bar has been raised since the patch first landed.

Comment 19

[Jon Coppeard (:jonco)]

The problem motivating this should have been fixed by bug 1634459.