Bug 187260 – WebCore: Fix clang static analyzer warnings: Assigned value is garbage or undefined

• Status: NEW
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

Fix "Assigned value is garbage or undefined" warnings in WebCore found by clang static analyzer deep analysis.

[Radar WebKit Bug Importer]

<rdar://problem/41735280>

[David Kilzer (:ddkilzer)]

Created attachment 344127 [details]
Patch v1

[EWS Watchlist]

Attachment 344127 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/CryptoAlgorithmIdentifier.h:33:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Comment on attachment 344127 [details]
Patch v1

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

> Source/WebCore/Modules/mediastream/RTCStatsReport.h:57
>          Type type;

Should type have a default value as well?

[David Kilzer (:ddkilzer)]

Comment on attachment 344127 [details]
Patch v1

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

>> Source/WebCore/Modules/mediastream/RTCStatsReport.h:57
>>          Type type;
> 
> Should type have a default value as well?

Yes, but there is no good "Invalid" option to use here as a default value.

I'm torn between using Type::Codec (which would have a value of zero), adding a new "Invalid" enum, or doing nothing and letting the static analyzer warn when a subclass (substruct?) fails to set it.

Thoughts?

[youenn fablet]

Comment on attachment 344127 [details]
Patch v1

LGTM, modulo the comment on VRDisplayEvent below.

(In reply to David Kilzer (:ddkilzer) from comment #5)
> Comment on attachment 344127 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344127&action=review
> 
> >> Source/WebCore/Modules/mediastream/RTCStatsReport.h:57
> >>          Type type;
> > 
> > Should type have a default value as well?
> 
> Yes, but there is no good "Invalid" option to use here as a default value.

And we cannot add one since it is bound to IDL.

> I'm torn between using Type::Codec (which would have a value of zero),
> adding a new "Invalid" enum, or doing nothing and letting the static
> analyzer warn when a subclass (substruct?) fails to set it.
> 
> Thoughts?

Ideally, the analyzer warning should be an error for that specific case.
If we cannot make it so, status quo seems fine to me.

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

> Source/WebCore/Modules/webvr/VRDisplayEvent.h:39
> +        VRDisplayEventReason reason { VRDisplayEventReason::Mounted };

Looking at this one, reason is not required, so it should either be std::optional<VRDisplayEventReason> or something like VRDisplayEventReason::None.

[David Kilzer (:ddkilzer)]

Comment on attachment 344127 [details]
Patch v1

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

>>>> Source/WebCore/Modules/mediastream/RTCStatsReport.h:57
>>>>          Type type;
>>> 
>>> Should type have a default value as well?
>> 
>> Yes, but there is no good "Invalid" option to use here as a default value.
>> 
>> I'm torn between using Type::Codec (which would have a value of zero), adding a new "Invalid" enum, or doing nothing and letting the static analyzer warn when a subclass (substruct?) fails to set it.
>> 
>> Thoughts?
> 
> And we cannot add one since it is bound to IDL.

I reran the clang static analyzer in deep mode, and I found that in JSRTCStatsReport.cpp, the various convertDictionary() methods can leave Stats.type and (below) IceCandidatePairStats.state undefined.

What are reasonable default values in each of these cases?  The ones that equate to the value zero?

[youenn fablet]

> > And we cannot add one since it is bound to IDL.
> 
> I reran the clang static analyzer in deep mode, and I found that in
> JSRTCStatsReport.cpp, the various convertDictionary() methods can leave
> Stats.type and (below) IceCandidatePairStats.state undefined.
> 
> What are reasonable default values in each of these cases?  The ones that
> equate to the value zero?

Would std::optional<Type> type do the trick?

[David Kilzer (:ddkilzer)]

(In reply to youenn fablet from comment #8)
> > > And we cannot add one since it is bound to IDL.
> > 
> > I reran the clang static analyzer in deep mode, and I found that in
> > JSRTCStatsReport.cpp, the various convertDictionary() methods can leave
> > Stats.type and (below) IceCandidatePairStats.state undefined.
> > 
> > What are reasonable default values in each of these cases?  The ones that
> > equate to the value zero?
> 
> Would std::optional<Type> type do the trick?

Yes.  We'd have to update the IDL for that change, or just change the instance variables?  (Sorry, I'm not up-to-date on the latest IDL support.)

[youenn fablet]

As long as the dictionary member is not required in the IDL, I would think we can use optional directly.

[David Kilzer (:ddkilzer)]

Comment on attachment 344127 [details]
Patch v1

Clearing r? flag until I can post a new patch.

[Sergio Villar Senin]

Comment on attachment 344127 [details]
Patch v1

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

>> Source/WebCore/Modules/webvr/VRDisplayEvent.h:39
>> +        VRDisplayEventReason reason { VRDisplayEventReason::Mounted };
> 
> Looking at this one, reason is not required, so it should either be std::optional<VRDisplayEventReason> or something like VRDisplayEventReason::None.

Youenn is right the reason is optional. However I'd advice not to change this because I'm going to modify it in a patch that is about to be uploaded for review.

[David Kilzer (:ddkilzer)]

(In reply to Sergio Villar Senin from comment #12)
> Comment on attachment 344127 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344127&action=review
> 
> >> Source/WebCore/Modules/webvr/VRDisplayEvent.h:39
> >> +        VRDisplayEventReason reason { VRDisplayEventReason::Mounted };
> > 
> > Looking at this one, reason is not required, so it should either be std::optional<VRDisplayEventReason> or something like VRDisplayEventReason::None.
> 
> Youenn is right the reason is optional. However I'd advice not to change
> this because I'm going to modify it in a patch that is about to be uploaded
> for review.

Okay.  Assuming this is Bug 187343.