[Static Analysis][Uninitialized scalar field] In function js::FutexRuntime::FutexRuntime from AtomicsObject.cpp

RESOLVED FIXED in Firefox 46

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
mozilla46
coverity
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: CID 1347691)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
The Static Analysis tool Coverity added that member variable canWait_ is not initialized in the default constructor of the class:

>>js::FutexRuntime::FutexRuntime()
>>  : cond_(nullptr),
>>    state_(Idle)
>>{
    	
CID 1347691 (#1 of 1): Uninitialized scalar field (UNINIT_CTOR)
2. uninit_member: Non-static class member canWait_ is not initialized in this constructor nor in any functions that it calls.
>>}

The above code it's a copy paste from Coverity Error log. Looking though code it's could be the case the canWait_ is read through:

>>bool canWait()

in

1.
>>js::FutexRuntime::wait(JSContext* cx, double timeout_ms, AtomicsObject::FutexWaitResult* result)
>>{
>>    MOZ_ASSERT(&cx->runtime()->fx == this);
>>    MOZ_ASSERT(cx->runtime()->fx.canWait());
>>    MOZ_ASSERT(lockHolder_ == PR_GetCurrentThread());

2. 
>>    if (!rt->fx.canWait())
>>        return ReportCannotWait(cx);
>>
>>    // This lock also protects the "waiters" field on SharedArrayRawBuffer,
>>    // and it provides the necessary memory fence.

Now the setter of the variable is only called through:

>>JS_SetFutexCanWait(JSRuntime* rt)
>>{
>>    rt->fx.setCanWait(true);
>>} 

and it get's caleld from:

>>    JSRuntime* rt = Runtime();
>>    MOZ_ASSERT(rt);
>>
>>    JS_SetRuntimePrivate(rt, new WorkerThreadRuntimePrivate(aWorkerPrivate));
>>
>>    js::SetPreserveWrapperCallback(rt, PreserveWrapper);
>>    JS_InitDestroyPrincipalsCallback(rt, DestroyWorkerPrincipals);
>>    JS_SetWrapObjectCallbacks(rt, &WrapObjectCallbacks);
>>    if (aWorkerPrivate->IsDedicatedWorker()) {
>>      JS_SetFutexCanWait(rt);

Now i can't figure for sure that setter gets called before the geter for canWait_, but surely can hurt to make it false in the constructor specially when the comment for this variable is:

>>    // If canWait() returns false (the default) then futexWait is disabled
>>    // on the runtime to which the FutexRuntime belongs.
(Assignee)

Comment 1

3 years ago
Created attachment 8706860 [details]
MozReview Request: Bug 1238911 - initialize canWait with false from constructor, avoid using the variable without initialization. r=lhansen

Review commit: https://reviewboard.mozilla.org/r/30519/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30519/
Attachment #8706860 - Flags: review?(jorendorff)
Comment on attachment 8706860 [details]
MozReview Request: Bug 1238911 - initialize canWait with false from constructor, avoid using the variable without initialization. r=lhansen

https://reviewboard.mozilla.org/r/30519/#review27243
Attachment #8706860 - Flags: review+
Comment on attachment 8706860 [details]
MozReview Request: Bug 1238911 - initialize canWait with false from constructor, avoid using the variable without initialization. r=lhansen

Stealing review, it's my fault :)
Attachment #8706860 - Flags: review?(jorendorff)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Attachment #8706860 - Attachment description: MozReview Request: Bug 1238911 - initialize canWait with false from constructor, avoid using the variable without initialization. r?jorendorff@mozilla.com → MozReview Request: Bug 1238911 - initialize canWait with false from constructor, avoid using the variable without initialization. r=lhansen
Attachment #8706860 - Flags: review?(jorendorff)
(Assignee)

Comment 4

3 years ago
Comment on attachment 8706860 [details]
MozReview Request: Bug 1238911 - initialize canWait with false from constructor, avoid using the variable without initialization. r=lhansen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30519/diff/1-2/
(Assignee)

Comment 5

3 years ago
https://reviewboard.mozilla.org/r/30519/#review27243

I've reupdated the patch since i've changed it's header in order to contain you as a reviewer. Hope this is ok.
(Assignee)

Comment 6

3 years ago
Comment on attachment 8706860 [details]
MozReview Request: Bug 1238911 - initialize canWait with false from constructor, avoid using the variable without initialization. r=lhansen

https://reviewboard.mozilla.org/r/30519/#review27247

Bug 1238911 - initialize canWait with false from constructor, avoid using the variable without initialization. r=lhansen
Attachment #8706860 - Flags: review+

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a7db157c7baa
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment on attachment 8706860 [details]
MozReview Request: Bug 1238911 - initialize canWait with false from constructor, avoid using the variable without initialization. r=lhansen

https://reviewboard.mozilla.org/r/30519/#review35483

I don't know how to get the review request removed from bugzilla except to review this again. Oh, ReviewBoard. :-|
Attachment #8706860 - Flags: review?(jorendorff) → review+
You need to log in before you can comment on or make changes to this bug.