Closed Bug 99324 Opened 18 years ago Closed 18 years ago

Plugin support code uses uninitialized variables to check plugin properties

Categories

(Core :: Plug-ins, defect)

x86
Windows 2000
defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: markh, Assigned: serhunt)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Blocks: 59652
Nudging this bug - it seems quite trivial, and once every blue moon may cause 
bad things to happen to pages with plugins.
Keywords: mozilla0.9.9
Attached patch New patch against the trunk (obsolete) — Splinter Review
Old patch still applied, but with 900 line offset using fuzz :)
Attachment #49071 - Attachment is obsolete: true
Instead of providing the default values, would not it be better to check whether
mInstance->GetValue returned an error? That would seem safer...
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 ;-)
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)?
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.
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.
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.
Attachment #70012 - Attachment is obsolete: true
Attachment #70024 - Attachment is obsolete: true
Comment on attachment 70235 [details] [diff] [review]
Updated patch that initializes the 3 identified uninitialized variables

Way to go, r=av
Attachment #70235 - Flags: review+
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
Attachment #70235 - Flags: superreview+
Comment on attachment 70235 [details] [diff] [review]
Updated patch that initializes the 3 identified uninitialized variables

a=scc
Attachment #70235 - Flags: approval+
> 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?
Checked in. Thanks all for taking care of the issue.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
verif patch was checked in.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.