Add ability to blacklist functions from UBSan and use it to suppress GC's generic calls
Categories
(Core :: JavaScript: GC, defect, P3)
Tracking
()
People
(Reporter: terrence, Assigned: terrence, NeedInfo)
References
(Blocks 1 open bug)
Details
(Keywords: triage-deferred)
Attachments
(1 file)
3.04 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fe87b0c8181
Comment 2•7 years ago
|
||
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. ;-)
Assignee | ||
Comment 3•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d71694334dc1
Assignee | ||
Comment 4•6 years ago
|
||
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]
Assignee | ||
Comment 5•6 years ago
|
||
GCC 4.8 is too old for UBSan, so I guess we'll need to just support clang for now.
Pushed by tcole@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d85334f696eb Blacklist UBSan detection of the GC's generic interfaces; r=sfink
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d85334f696eb
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d85334f696eb
Comment 9•6 years ago
|
||
The patch doesn't compile for me. Mac 10.10.5, Xcode 7.2.1
Comment 11•6 years ago
|
||
/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
Assignee | ||
Comment 12•6 years ago
|
||
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.
Assignee | ||
Comment 13•6 years ago
|
||
I guess we are still on 3.6, so here is a backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/16daeb9b4e17
Comment 14•6 years ago
|
||
reopen because of backout as mentioned in #13
Comment 15•6 years ago
|
||
You just need my patch in bug 1305345.
Updated•5 years ago
|
![]() |
||
Comment 16•3 years ago
|
||
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•3 years ago
|
||
(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•3 years ago
|
||
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•3 years ago
|
||
The problem motivating this should have been fixed by bug 1634459.
Description
•