Plugin support code uses uninitialized variables to check plugin properties

VERIFIED FIXED

Status

()

Core
Plug-ins
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: markh, Assigned: av (gone))

Tracking

Trunk
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

16 years ago
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.
(Reporter)

Comment 1

16 years ago
Created attachment 49071 [details] [diff] [review]
Proposed patch that initializes the variables.

Updated

16 years ago
Blocks: 59652
(Reporter)

Comment 2

16 years ago
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
(Reporter)

Comment 3

16 years ago
Created attachment 70012 [details] [diff] [review]
New patch against the trunk

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

Updated

16 years ago
Attachment #49071 - Attachment is obsolete: true

Comment 4

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

Comment 5

16 years ago
Created attachment 70024 [details] [diff] [review]
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

16 years ago
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)?
(Reporter)

Comment 7

16 years ago
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

16 years ago
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.
(Reporter)

Comment 9

16 years ago
Created attachment 70235 [details] [diff] [review]
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.
Attachment #70012 - Attachment is obsolete: true
Attachment #70024 - Attachment is obsolete: true
(Assignee)

Comment 10

16 years ago
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 11

16 years ago
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 12

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

a=scc
Attachment #70235 - Flags: approval+
(Reporter)

Comment 13

16 years ago
> 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?
(Assignee)

Comment 14

16 years ago
Checked in. Thanks all for taking care of the issue.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 15

15 years ago
verif patch was checked in.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.