Closed Bug 1285057 Opened 7 years ago Closed 3 years ago

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


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




Tracking Status
firefox50 --- affected
firefox52 --- fixed


(Reporter: terrence, Assigned: terrence, NeedInfo)


(Blocks 1 open bug)


(Keywords: triage-deferred)


(1 file)

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.
Attachment #8768593 - Flags: review?(sphink)
Comment on attachment 8768593 [details] [diff] [review]

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))
# define MOZ_UBSAN_BLACKLIST_FUNCTION /* nothing */

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. ;-)
Attachment #8768593 - Flags: review?(sphink) → review+
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]
GCC 4.8 is too old for UBSan, so I guess we'll need to just support clang for now.
Pushed by
Blacklist UBSan detection of the GC's generic interfaces; r=sfink
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
The patch doesn't compile for me. Mac 10.10.5, Xcode 7.2.1
What are the failures?
Flags: needinfo?(jvarga)
/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
Flags: needinfo?(jvarga)
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.
I guess we are still on 3.6, so here is a backout:
Depends on: 1305345
reopen because of backout as mentioned in #13
Flags: needinfo?(terrence.d.cole)
Resolution: FIXED → ---
You just need my patch in bug 1305345.
Keywords: triage-deferred
Priority: -- → P3

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

See Also: → 1587173

(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.

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.

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

Closed: 6 years ago3 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.