1592875 - nsPluginTag::GetEnabledState accesses an uninitialized value by missing a proper return value check of GetInt()

Status: RESOLVED WORKSFORME

(Core Graveyard :: Plug-ins, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: add-missing-result-check.patch

nsPluginTag::GetEnabledState accesses an uninitialized value by missing a proper check of the return value of GetInt() in one location.

Noticed by running C-C TB xpcshell-tests under valgrind:
The error message from valgrind:

880:06.75 pid:31803 ==31803== Conditional jump or move depends on uninitialised value(s)
880:06.75 pid:31803 ==31803==    at 0x9584FB6: nsPluginTag::GetEnabledState(unsigned int*) (nsPluginTags.cpp:522)
880:06.75 pid:31803 ==31803==    by 0x958ADAA: nsPluginTag::IsActive() (nsPluginTags.cpp:564)
880:06.75 pid:31803 ==31803==    by 0x957C618: nsPluginHost::AddPluginTag(nsPluginTag*) (nsPluginHost.cpp:1931)
880:06.75 pid:31803 ==31803==    by 0x957A1DF: nsPluginHost::ScanPluginsDirectory(nsIFile*, bool, bool*) (nsPluginHost.cpp:2110)
880:06.75 pid:31803 ==31803==    by 0x957A5CC: nsPluginHost::ScanPluginsDirectoryList(nsISimpleEnumerator*, bool, bool*) (nsPluginHost.cpp:2151)
880:06.75 pid:31803 ==31803==    by 0x957A979: nsPluginHost::FindPlugins(bool, bool*) (nsPluginHost.cpp:2353)
880:06.75 pid:31803 ==31803==    by 0x957B171: nsPluginHost::LoadPlugins() (nsPluginHost.cpp:2220)
880:06.75 pid:31803 ==31803==    by 0x957B517: nsPluginHost::nsPluginHost() (nsPluginHost.cpp:408)
880:06.75 pid:31803 ==31803==    by 0x957B5A4: nsPluginHost::GetInst() (nsPluginHost.cpp:423)
           ,,,

Line 522 of nsPluginTags.cpp:

https://dxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginTags.cpp?redirect_type=direct#522

NS_IMETHODIMP
nsPluginTag::GetEnabledState(uint32_t* aEnabledState) {
  int32_t enabledState;
  nsresult rv =
      Preferences::GetInt(GetStatePrefNameForPlugin(this).get(), &enabledState);
  if (enabledState == nsIPluginTag::STATE_ENABLED && mIsFlashPlugin) {  <=== THIS 
    enabledState = nsIPluginTag::STATE_CLICKTOPLAY;
  }
  if (NS_SUCCEEDED(rv) && enabledState >= nsIPluginTag::STATE_DISABLED &&
      enabledState <= nsIPluginTag::STATE_ENABLED) {
    *aEnabledState = (uint32_t)enabledState;
    return rv;
  }

A couple of lines below the line 522, we see that NS_SUCCEEDED(rv) precedes the
if expressions so that uninitialized |enabledState| is not accessed.

So I think it is appropriate to add |NS_SUCCEEDED(rv) &&| at the beginning of the if expression on the line 522.

The patch is attached.

The patch is checked by building and running test of C-C TB.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ad2f5c55f9e9ffca259cdcde5b761f3d1fb570f3

Comment 1

[ISHIKAWA, Chiaki]

So whom shoud I request to review this?

Comment 2

[ISHIKAWA, Chiaki]

At least the patch builds and produces no additional errors for C-C TB build and test.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e1c9f00a40034361a9a3b6c4abdf0807e22c5d7e

I appreciate if someone can apply this to M-C FF and test this. I believe the build succeeds obviously.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[ISHIKAWA, Chiaki]

The code seems to have changed, and the logic, too.
So the patch no longer applies.

I am setting this as RESOLVED WORKSFORME.
If a new valgrind error/warning is found near here, I will file a new bugzilla.