1587176 - call to function mozilla::pref_CompareFileNames(nsIFile*, nsIFile*, void*) through pointer to incorrect function type in xpcom/ds/nsCOMArray.cpp:103

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Tyson Smith [:tsmith]]

This is triggered on start up with an UBSan build. To enable this check add the following to your mozconfig:

ac_add_options --enable-address-sanitizer
ac_add_options --enable-undefined-sanitizer="function"
ac_add_options --disable-jemalloc

This issue can be triggered by attempting to launch the browser.

xpcom/ds/nsCOMArray.cpp:103:10: runtime error: call to function mozilla::pref_CompareFileNames(nsIFile*, nsIFile*, void*) through pointer to incorrect function type 'int (*)(nsISupports *, nsISupports *, void *)'
modules/libpref/Preferences.cpp:4184: note: mozilla::pref_CompareFileNames(nsIFile*, nsIFile*, void*) defined here
    #0 0x7fdb7fa9f94f in nsCOMArray_base::nsCOMArrayComparator(void const*, void const*, void*) xpcom/ds/nsCOMArray.cpp:103:10
    #1 0x7fdb7fad1079 in NS_QuickSort xpcom/ds/nsQuickSort.cpp:105:38
    #2 0x7fdb7fa9fab4 in nsCOMArray_base::Sort(int (*)(nsISupports*, nsISupports*, void*), void*) xpcom/ds/nsCOMArray.cpp:111:5
    #3 0x7fdb7fd1c6c1 in nsCOMArray<nsIFile>::Sort(int (*)(nsIFile*, nsIFile*, void*), void*) objdir-ff-ubsan/dist/include/nsCOMArray.h:304:22
    #4 0x7fdb7fd1c6c1 in mozilla::pref_LoadPrefsInDir(nsIFile*, char const* const*, unsigned int) modules/libpref/Preferences.cpp:4258
    #5 0x7fdb7fcb54a9 in mozilla::Preferences::InitInitialObjects(bool) modules/libpref/Preferences.cpp:4612:7
    #6 0x7fdb7fcb376a in mozilla::Preferences::GetInstanceForService() modules/libpref/Preferences.cpp:3422:17
    #7 0x7fdb7fba33bc in mozilla::xpcom::CreateInstanceImpl(mozilla::xpcom::ModuleID, nsISupports*, nsID const&, void**) objdir-ff-ubsan/xpcom/components/StaticComponents.cpp:9548:43
    #8 0x7fdb7fbd35b3 in (anonymous namespace)::EntryWrapper::CreateInstance(nsISupports*, nsID const&, void**) xpcom/components/nsComponentManager.cpp:224:46
    #9 0x7fdb7fbd35b3 in nsComponentManagerImpl::GetServiceLocked((anonymous namespace)::MutexLock&, (anonymous namespace)::EntryWrapper&, nsID const&, void**) xpcom/components/nsComponentManager.cpp:1383
    #10 0x7fdb7fbca594 in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) xpcom/components/nsComponentManager.cpp:1570:10
    #11 0x7fdb7fbdb69e in CallGetService(char const*, nsID const&, void**) xpcom/components/nsComponentManagerUtils.cpp:61:43
    #12 0x7fdb7fbdb69e in nsGetServiceByContractID::operator()(nsID const&, void**) const xpcom/components/nsComponentManagerUtils.cpp:243
    #13 0x7fdb7fa22e28 in nsCOMPtr_base::assign_from_gs_contractid(nsGetServiceByContractID, nsID const&) xpcom/base/nsCOMPtr.cpp:82:7
    #14 0x7fdb7fd365f2 in nsCOMPtr<nsIPrefService>::nsCOMPtr(nsGetServiceByContractID) objdir-ff-ubsan/dist/include/nsCOMPtr.h:607:5
    #15 0x7fdb7fd365f2 in mozilla::Preferences::InitStaticMembers() modules/libpref/Preferences.cpp:3504
    #16 0x7fdb7fd365f2 in nsresult mozilla::Preferences::RegisterCallbackImpl<char const**>(void (*)(char const*, void*), char const**&, void*, mozilla::Preferences::MatchKind, bool) modules/libpref/Preferences.cpp:4992
    #17 0x7fdb7fd21175 in mozilla::Preferences::RegisterCallbacks(void (*)(char const*, void*), char const**, void*, mozilla::Preferences::MatchKind) modules/libpref/Preferences.cpp:5029:10
    #18 0x7fdb81cd5236 in nsresult mozilla::Preferences::RegisterCallbacks<WatchdogManager>(mozilla::TypedPrefChangeFunc<WatchdogManager>::Type, char const**, WatchdogManager*) objdir-ff-ubsan/dist/include/mozilla/Preferences.h:393:12
    #19 0x7fdb81cd5236 in WatchdogManager::WatchdogManager() js/xpconnect/src/XPCJSContext.cpp:270
    #20 0x7fdb81cd5236 in XPCJSContext::GetWatchdogManager() js/xpconnect/src/XPCJSContext.cpp:1285
    #21 0x7fdb81cd4e9b in XPCJSContext::XPCJSContext() js/xpconnect/src/XPCJSContext.cpp:1039:24
    #22 0x7fdb81cd661b in XPCJSContext::NewXPCJSContext(XPCJSContext*) js/xpconnect/src/XPCJSContext.cpp:1294:28
    #23 0x7fdb81d85376 in nsXPConnect::nsXPConnect() js/xpconnect/src/nsXPConnect.cpp:75:25
    #24 0x7fdb81d8572c in nsXPConnect::InitStatics() js/xpconnect/src/nsXPConnect.cpp:127:15
    #25 0x7fdb81d06840 in xpcModuleCtor() js/xpconnect/src/XPCModule.cpp:11:3
    #26 0x7fdb890906a0 in nsLayoutModuleInitialize() layout/build/nsLayoutModule.cpp:107:7
    #27 0x7fdb7fbcb47b in nsComponentManagerImpl::Init() xpcom/components/nsComponentManager.cpp:493:5
    #28 0x7fdb7fc956a1 in NS_InitXPCOM xpcom/build/XPCOMInit.cpp:445:51
    #29 0x7fdb8c2a8c66 in ScopedXPCOMStartup::Initialize() toolkit/xre/nsAppRunner.cpp:1281:8
    #30 0x7fdb8c2be41d in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) toolkit/xre/nsAppRunner.cpp:4731:22
    #31 0x7fdb8c2bfb6b in XRE_main(int, char**, mozilla::BootstrapConfig const&) toolkit/xre/nsAppRunner.cpp:4816:21
    #32 0x564a8fdff145 in do_main(int, char**, char**) browser/app/nsBrowserApp.cpp:218:22
    #33 0x564a8fdfe3ff in main browser/app/nsBrowserApp.cpp:300:16

Comment 1

[(Away)]

nsCOMArray_base::Sort wants to deal with pointers to functions taking nsISupports* parameters, but nsCOMArray<T>::Sort calls the base with pointers to functions taking (say) nsIFile*.

Seems like "it ought to work" but I guess the function pointer rules don't allow for covariance?

Should we template the base's Sort method? Rewrite all the callers to use just nsISupports* and downcast everywhere? I don't know. Maybe froydnj has an opinion here.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to :dmajor from comment #1)

nsCOMArray_base::Sort wants to deal with pointers to functions taking nsISupports* parameters, but nsCOMArray<T>::Sort calls the base with pointers to functions taking (say) nsIFile*.

Seems like "it ought to work" but I guess the function pointer rules don't allow for covariance?

It does (mostly) work because (uintptr_t)static_cast<nsISupports*>(file) == file.

Should we template the base's Sort method? Rewrite all the callers to use just nsISupports* and downcast everywhere? I don't know. Maybe froydnj has an opinion here.

I think instead of templating, I'd implement nsCOMArray<T>::Sort and have that use a forwarding comparison function that does the appropriate downcasting. For this particular issue, I think we could just make preferences not use nsCOMArray.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Bug 1587176 - Fix UBSAN complaints about pref callbacks. r=erahm

Lots of these callbacks have a non-void* final parameter, which UBSAN
complains about. This commit changes them to have a void* parameter.

This requires undoing the machinery added in the first two commits of bug
1473631: TypePrefChangeFunc and PREF_CHANGE_METHOD. The resulting code is
simpler (which is good) and more boilerplate-y (which is bad) but avoids the
undefined behaviour (which is good).

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Bug 1587176 - Rename some nsCOMArray internal stuff. r=erahm

This makes the subsequent commit easier to understand.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Bug 1587176 - Fix UBSAN complaints about nsCOMArray. r=erahm

nsCOMArray currently casts a function that takes two void pointers into a
function that takes two nsISupports pointers. UBSAN doesn't like this.

This commit introduces an extra level of casting machinery. As a result, each
comparison done while sorting goes through an extra level, to get from (T*, T*) comparisons to (nsISupports*, nsISupports*) comparisons to (void*, void*) comparisons, as required by NS_QuickSort(). It's a bit annoying but I
can't see how else to solve this within the constraint imposed by the existing
nsCOMArray_base/nsCOMArray split.

Depends on D51222

Comment 6

[Nicholas Nethercote [inactive]]

Whoops, the "Fix UBSAN complaints about pref callbacks" commit was supposed to go to bug 1587162. I will move it there now.

Comment 7

[Phabricator Automation]

Comment on attachment 9104832 [details]
Bug 1587176 - Fix UBSAN complaints about pref callbacks. r=erahm

Revision D50901 was moved to bug 1587162. Setting attachment 9104832 [details] to obsolete.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76e178373052
Rename some nsCOMArray internal stuff. r=erahm
https://hg.mozilla.org/integration/autoland/rev/21d7aafd9e32
Fix UBSAN complaints about nsCOMArray. r=erahm

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/76e178373052
https://hg.mozilla.org/mozilla-central/rev/21d7aafd9e32