99324 - Plugin support code uses uninitialized variables to check plugin properties

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Code in nsObjectFrame.cpp attempts to query the plugin for special 
variables "nsPluginInstanceVariable_WindowlessBool" 
and "nsPluginInstanceVariable_TransparentBool".  It queries these values 
without pre-initializing the result value.

No plug-ins provide default values for unknown variable values.  Thus, for 
plugins that do not understand these values (the vast majority, including all 
sample plugins), the value remains uninitialized.

This code still works in the vast majority of cases, as any value other than 
exactly PR_TRUE will be rejected, and the random uninitialized value is 
unlikely to be exactly PR_TRUE.  However, it *will* happen at some stage, 
causing plugins to be considered "windowless", and therefore will not be 
displayed.

Patch that initializes these values follows.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Proposed patch that initializes the variables.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Nudging this bug - it seems quite trivial, and once every blue moon may cause 
bad things to happen to pages with plugins.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: New patch against the trunk

Old patch still applied, but with 900 line offset using fuzz :)

Comment 4

[Aleksey Nogin]

Instead of providing the default values, would not it be better to check whether
mInstance->GetValue returned an error? That would seem safer...

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Attachment: Updated patch that checks return code of GetValue

Not sure I agree 100% that NS_SUCCESS will always imply a correctly initialized
value.	But here is a patch anyway.  I have chosen not to obsolete the old one
incase a reviewer happens to agree with me ;-)

Comment 6

[Aleksey Nogin]

Actually, this is not what I meant. What I meant is - if GetValue fails, is it
wise to just fall back to a defaultvalue and continue? Wouldn't it be safer to
just abort (e.g. immediatelly return whatever GetValue returned)?

Comment 7

[Mark Hammond [:markh] [:mhammond]]

No - we can not abort in this case.  All GetValue() implementations only do 
something sensible for values they directly support.  Eg, if you look in the 
sample plugin source code, you will see that NS_ERROR_NOT_IMPLEMENTED or 
NS_ERROR_FAILURE are common values.  NS_SUCCESS is only returned when the 
specific variable name is directly supported - and in the sample plugins these 
variables never are.  Therefore, failure will be returned for *all* of the 
plugin samples, and almost all plugins that exist.

With the code in question, it is obvious to me that aborting out is *not* an 
option.  These plugins must allow processing to continue with the default value 
of FALSE (ie, not transparent and not windowless).  Insisting that all plugins 
support these variables is also not an option for b/w compatability.

Comment 8

[David Brittain]

There is one other place where the variable is uninitialised on line 1672. I
would add it into the patch but my build is broken at the moment.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Attachment: Updated patch that initializes the 3 identified uninitialized variables

Updated the patch to include the extra variable caught by David.  Reverted back
to the original patch style - don't check the GetValue() return code - just
init the value before making the call.

Comment 10

[av (gone)]

Comment on attachment 70235 [details] [diff] [review]
Updated patch that initializes the 3 identified uninitialized variables

Way to go, r=av

Comment 11

[Patrick C. Beard]

Comment on attachment 70235 [details] [diff] [review]
Updated patch that initializes the 3 identified uninitialized variables

These technically aren't unitialized variables, since they are out parameters,
so the initialization is pointless.
sr=beard

Comment 12

[Scott Collins]

Comment on attachment 70235 [details] [diff] [review]
Updated patch that initializes the 3 identified uninitialized variables

a=scc

Comment 13

[Mark Hammond [:markh] [:mhammond]]

> These technically aren't unitialized variables, since they are 
> out parameters, so the initialization is pointless

I'm not sure.  I am fairly certain I discovered this in the debugger rather 
than by examination - ie, the uninitialized value *is* a problem.

Note that most call sites are C++ implemented, and AFAIK, there is no magic 
involved that will cause these out params to be automatically intialized.

But whatever :)

av - you OK to check this in?

Comment 14

[av (gone)]

Checked in. Thanks all for taking care of the issue.

Comment 15

[shrirang khanzode]

verif patch was checked in.