1413063 - UBSan: js/src/gc/Nursery.cpp:486:20: runtime error: division by zero [@ calcPromotionRate]

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

This is triggered on start up when running Firefox built with "-fsanitize=float-divide-by-zero,integer-divide-by-zero"

/mozilla-central/js/src/gc/Nursery.cpp:486:20: runtime error: division by zero
    #0 0x7f65721a406e in calcPromotionRate /mozilla-central/js/src/gc/Nursery.cpp:486:20
    #1 0x7f65721a406e in js::Nursery::collect(JS::gcreason::Reason) /mozilla-central/js/src/gc/Nursery.cpp:652
    #2 0x7f657176680a in js::gc::GCRuntime::minorGC(JS::gcreason::Reason, js::gcstats::PhaseKind) /mozilla-central/js/src/jsgc.cpp:7687:15
    #3 0x7f657176aec5 in evictNursery /mozilla-central/js/src/gc/GCRuntime.h:1522:9
    #4 0x7f657176aec5 in JS::AutoDisableGenerationalGC::AutoDisableGenerationalGC(JSContext*) /mozilla-central/js/src/jsgc.cpp:7708
    #5 0x7f6571c6054a in JSRuntime::initSelfHosting(JSContext*) /mozilla-central/js/src/vm/SelfHosting.cpp:2872:35
    #6 0x7f6571641a03 in JS::InitSelfHostedCode(JSContext*) /mozilla-central/js/src/jsapi.cpp:660:14
    #7 0x7f6566347152 in nsXPConnect::InitStatics() /mozilla-central/js/xpconnect/src/nsXPConnect.cpp:135:10
    #8 0x7f65662bac98 in xpcModuleCtor() /mozilla-central/js/xpconnect/src/XPCModule.cpp:13:5
    #9 0x7f656cdb49e7 in Initialize() /mozilla-central/layout/build/nsLayoutModule.cpp:316:8
    #10 0x7f65642d2cad in nsComponentManagerImpl::KnownModule::Load() /mozilla-central/xpcom/components/nsComponentManager.cpp:763:21
    #11 0x7f65642d3f61 in nsFactoryEntry::GetFactory() /mozilla-central/xpcom/components/nsComponentManager.cpp:1785:19
    #12 0x7f65642d5356 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /mozilla-central/xpcom/components/nsComponentManager.cpp:1083:41
    #13 0x7f65642cccef in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) /mozilla-central/xpcom/components/nsComponentManager.cpp:1446:10
    #14 0x7f65642dc021 in CallGetService /mozilla-central/xpcom/components/nsComponentManagerUtils.cpp:67:43
    #15 0x7f65642dc021 in nsGetServiceByContractID::operator()(nsID const&, void**) const /mozilla-central/xpcom/components/nsComponentManagerUtils.cpp:280
    #16 0x7f6564178ae5 in nsCOMPtr_base::assign_from_gs_contractid(nsGetServiceByContractID, nsID const&) /mozilla-central/xpcom/base/nsCOMPtr.cpp:95:7
    #17 0x7f656438f764 in nsCOMPtr /mozilla-central/objdir-ff-ubsan/dist/include/nsCOMPtr.h:928:5
    #18 0x7f656438f764 in NS_InitXPCOM2 /mozilla-central/xpcom/build/XPCOMInit.cpp:709
    #19 0x7f657090b276 in Initialize /mozilla-central/toolkit/xre/nsAppRunner.cpp:1573:8
    #20 0x7f657090b276 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /mozilla-central/toolkit/xre/nsAppRunner.cpp:4844
    #21 0x7f657090ceb5 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /mozilla-central/toolkit/xre/nsAppRunner.cpp:4943:21
    #22 0x516f37 in do_main /mozilla-central/browser/app/nsBrowserApp.cpp:231:22
    #23 0x516f37 in main /mozilla-central/browser/app/nsBrowserApp.cpp:304
    #24 0x7f65858fa1c0 in __libc_start_main /build/glibc-CxtIbX/glibc-2.26/csu/../csu/libc-start.c:308
    #25 0x41f749 in _start (/mozilla-central/objdir-ff-ubsan/dist/bin/firefox+0x41f749)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /mozilla-central/js/src/gc/Nursery.cpp:486:20 in

Comment 1

[Jon Coppeard (:jonco)]

Paul, I think you added this method.  Can you take a look?

Comment 2

[Paul Bone [:pbone]]

Yes, this is my code. I'll look today.

Comment 3

[Paul Bone [:pbone]]

Attachment: Bug 1413063 - Avoid divide by zero

I was able to reproduce this locally but I have started a try job anyway.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4db824b8af6cfa91939926191d56f2aa6df93c27&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable

Comment 4

[Paul Bone [:pbone]]

This code was introduced in Bug 1392511 which is in 57.  However in the case that there is a division by zero the result is not used because *validForTenuring is set to false.  Therefore I don't think this needs an uplift, but someone else may disagree.

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 8924055 [details] [diff] [review]
Bug 1413063 - Avoid divide by zero

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

::: js/src/gc/Nursery.cpp
@@ +484,5 @@
> +             */
> +            *validForTenuring = used > capacity * 0.9f;
> +        }
> +        return tenured / used;
> +    } else {

According to the coding style we should avoid an else block after a then block that ends in return, to simplify the control flow.

Comment 6

[Paul Bone [:pbone]]

(In reply to Jon Coppeard (:jonco) from comment #5)
> Comment on attachment 8924055 [details] [diff] [review]
> Bug 1413063 - Avoid divide by zero
> 
> Review of attachment 8924055 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/gc/Nursery.cpp
> @@ +484,5 @@
> > +             */
> > +            *validForTenuring = used > capacity * 0.9f;
> > +        }
> > +        return tenured / used;
> > +    } else {
> 
> According to the coding style we should avoid an else block after a then
> block that ends in return, to simplify the control flow.

Okay I'll fix this. It's weird to me from a functional programming perspective but that's okay.

Comment 7

[Paul Bone [:pbone]]

Attachment: Bug 1413063 - Avoid divide by zero r=jonco

Updated based on review feedback. r+ carried forward.

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

Comment 8

[Paul Bone [:pbone]]

Hi Tyson,

Should we uplift this onto 57?

Not that there shouldn't be a problem on 57 because the result of the divide-by-zero isn't used. However it is still undefined behaviour.

Thanks.

Comment 9

[Tyson Smith [:tsmith]]

Hi Paul, thanks for the fix!

> Should we uplift this onto 57?
On one hand UB is bad, whether the value is used or not other (potentially less obvious) bugs can be created depending on assumptions the compiler is free to make while compiling. On the other hand it is very close to release so I'd say no unless we are seeing issues we are fairly certain are related. In the end it's up to the release managers.

Comment 10

[Paul Bone [:pbone]]

Yep, those were my thoughts as well. Thanks for raising this further.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Dan, Naveed, would you be able to weigh in on this one? Would you consider this a must fix for 57? Based on comment 4, it doesn't sound like a must fix.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b34ad88764a9
Avoid divide by zero. r=jonco

Comment 13

[Narcis Beleuzu [:NarcisB]]

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

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Up to you, but I would say wontfix for 57.

Comment 15

[Tyson Smith [:tsmith]]

I also vote wontfix for 57.

Comment 16

[Paul Bone [:pbone]]

It looks like this is wontfix, and 57's beta window is closing so I'm going to close this now.