Closed Bug 1587176 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox71 --- wontfix
firefox72 --- fixed

People

(Reporter: tsmith, Assigned: n.nethercote)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-undefined)

Attachments

(2 files, 1 obsolete file)

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

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.

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

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

This makes the subsequent commit easier to understand.

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

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

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.

Attachment #9104832 - Attachment is obsolete: true
Attachment #9105465 - Attachment description: Bug 1587176 - Fix UBSAN complaints about nsCOMArray. → Bug 1587176 - Fix UBSAN complaints about nsCOMArray. r=erahm
Pushed by nnethercote@mozilla.com: 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
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Assignee: nobody → n.nethercote
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: