1006307 - widget/xpwidgets/GfxInfoBase.cpp:450:43: error: 'version' may be used uninitialized in function BlacklistEntryToDriverInfo() [-Wuninitialized]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Chris Peterson [:cpeterson]]

ParseDriverVersion() returns true/success without setting out-parameter aNumericVersion #if !defined(XP_WIN) && !defined(ANDROID). This is a regression from bug 689598.

widget/xpwidgets/GfxInfoBase.cpp: In member function 'virtual nsresult mozilla::widget::GfxInfoBase::Observe(nsISupports*, const char*, const char16_t*)':
widget/xpwidgets/GfxInfoBase.cpp:450:43: error: 'version' may be used uninitialized in this function [-Werror=uninitialized]
widget/xpwidgets/GfxInfoBase.cpp:448:14: note: 'version' was declared here

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: ParseDriverVersion_Wuninitialized.patch

Fix -Wuninitialized and -Wunused-variable warnings in widget/xpwidgets/GfxInfoBase.cpp.

Should ParseDriverVersion() return true/success or false on platforms other than Windows and Android? btw, of the eight calls to ParseDriverVersion() in the tree, only two actually check the return value.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8418568 [details] [diff] [review]
ParseDriverVersion_Wuninitialized.patch

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

It's good to return false when not supporting driver version parsing. This seems to be only used in BlacklistEntryToDriverInfo(), which seems to expect that.

::: widget/xpwidgets/GfxInfoBase.cpp
@@ +599,3 @@
>    uint64_t driverVersion;
>    ParseDriverVersion(adapterDriverVersionString, &driverVersion);
> +#endif

Actually driverVersion seems to be unused in this function. So you can just remove this two lines, like the other two lines below in this file.

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Benoit Jacob [:bjacob] from comment #2)
> ::: widget/xpwidgets/GfxInfoBase.cpp
> @@ +599,3 @@
> >    uint64_t driverVersion;
> >    ParseDriverVersion(adapterDriverVersionString, &driverVersion);
> > +#endif
> 
> Actually driverVersion seems to be unused in this function. So you can just
> remove this two lines, like the other two lines below in this file.

driverVersion is unused in GfxInfoBase::GetFeatureStatusImpl(), but it is used in GfxInfoBase::FindBlocklistedDeviceInList #if defined(XP_WIN) || defined(ANDROID):

https://hg.mozilla.org/mozilla-central/file/fef1a56f0237/widget/xpwidgets/GfxInfoBase.cpp#l650

Comment 4

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f5a03f07f9a

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1f5a03f07f9a

Comment 6

[Chris Peterson [:cpeterson]]

Comment on attachment 8418568 [details] [diff] [review]
ParseDriverVersion_Wuninitialized.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: No known issues, but this patch fixes possible uninitialized data when Firefox tries to read the graphics driver version, which could cause graphics driver blacklist confusion.
Testing completed (on m-c, etc.): m-c for four days
Risk to taking this patch (and alternatives if risky): Low risk.
String or IDL/UUID changes made by this patch: No string changes.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/64d26e693baa

Comment 8

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

Chris, is this something you can include in a testsuite or is there some way QA can help out? Thanks!

Comment 9

[Chris Peterson [:cpeterson]]

(In reply to Liz Henry :lizzard from comment #8)
> Chris, is this something you can include in a testsuite or is there some way
> QA can help out? Thanks!

No. We don't have any relevant tests for this particular fix.