Bug 211267

Summary: REGRESSION (r154253): JSC::PropertySlot::m_attributes is uninitialized in constructor
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: JavaScriptCoreAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 119972    
Bug Blocks: 212095    
Attachments:
Description Flags
Patch v1 none

Description David Kilzer (:ddkilzer) 2020-04-30 17:38:23 PDT
JSC::PropertySlot::m_attributes is uninitialized in constructor.

Found by clang static analyzer with optin.cplusplus.UninitializedObject checker enabled.
Comment 1 David Kilzer (:ddkilzer) 2020-04-30 17:39:04 PDT
Created attachment 398128 [details]
Patch v1
Comment 2 Radar WebKit Bug Importer 2020-04-30 17:40:15 PDT
<rdar://problem/62687958>
Comment 3 Mark Lam 2020-04-30 17:42:20 PDT
Comment on attachment 398128 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=398128&action=review

r=me

> Source/JavaScriptCore/runtime/PropertySlot.h:404
> +    } m_additionalData { { 0, 0 } };

Interesting.  I never knew that we can do this.
Comment 4 David Kilzer (:ddkilzer) 2020-04-30 17:51:22 PDT
Comment on attachment 398128 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=398128&action=review

>> Source/JavaScriptCore/runtime/PropertySlot.h:404
>> +    } m_additionalData { { 0, 0 } };
> 
> Interesting.  I never knew that we can do this.

I think it works because both union types have two instance variables.  I guess gcc and MSVC++ will tell us if it's portable, though.
Comment 5 David Kilzer (:ddkilzer) 2020-04-30 17:53:53 PDT
Regressed in:

    Bug 119972: Add attributes field to PropertySlot
    <https://bugs.webkit.org/show_bug.cgi?id=119972>
    <https://trac.webkit.org/r154253>
Comment 6 EWS 2020-05-01 03:26:15 PDT
Committed r260993: <https://trac.webkit.org/changeset/260993>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398128 [details].
Comment 7 Darin Adler 2020-05-05 18:16:46 PDT
Comment on attachment 398128 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=398128&action=review

>>> Source/JavaScriptCore/runtime/PropertySlot.h:404
>>> +    } m_additionalData { { 0, 0 } };
>> 
>> Interesting.  I never knew that we can do this.
> 
> I think it works because both union types have two instance variables.  I guess gcc and MSVC++ will tell us if it's portable, though.

No, it’s only domAttribute that is initialized. I looked it up:

"When a union is initialized by aggregate initialization, only its first non-static data member is initialized."
Comment 8 David Kilzer (:ddkilzer) 2020-05-19 11:30:30 PDT
(In reply to Darin Adler from comment #7)
> Comment on attachment 398128 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398128&action=review
> 
> >>> Source/JavaScriptCore/runtime/PropertySlot.h:404
> >>> +    } m_additionalData { { 0, 0 } };
> >> 
> >> Interesting.  I never knew that we can do this.
> > 
> > I think it works because both union types have two instance variables.  I guess gcc and MSVC++ will tell us if it's portable, though.
> 
> No, it’s only domAttribute that is initialized. I looked it up:
> 
> "When a union is initialized by aggregate initialization, only its first
> non-static data member is initialized."

Bug 212095: Make union initializers for JSC::PropertySlot more explicit about which field is being initialized