493545 - malformed pluginreg.dat causes a crash [@ nsPluginFile::LoadPlugin ]

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Radek 'sysKin' Czyz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090517 Firefox/3.6a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090517 Minefield/3.6a1pre

Plugin loader assumes that a plugin path has a backspace symbol in it. If it doesn't, it crashes.
http://hg.mozilla.org/mozilla-central/annotate/bff114502666/modules/plugin/base/src/nsPluginsDirWin.cpp#l230

Looks like it needs a null check before line l231.


Reproducible: Always

Steps to Reproduce:
1. Make a malformed pluginreg.dat that doesn't have a backslash symbol in plugin's path
2. Load any page with plugins

Actual Results:  
bp-412d8f48-f5cb-4ffc-8430-b0ba22090517 and friends

Expected Results:  
faulty profile data should not cause a crash :)


I managed to get to this state by running a 20090516 build (with bug 488181's patch checked in), suffering from bug 493375 and "solving" it by reinstalling flash. Then, the following nightly had bug 488181's patch backed out and *boom*.

Comment 1

[Timothy Nikkel (:tnikkel)]

Confirmed, happens to me too.

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 3

[timeless]

so. i'd rather we looked for the \ before we allocated, that'd simplify the code (fewer paths to release memory).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 378262 [details] [diff] [review]
patch

Looks good.  I prefer this to checking before allocating, because this way we're checking he same thing as we'll pass to people (and in particular, if |temp| has embedded nulls, checking for '\\' in |temp| might not give us what we want).

Comment 5

[timeless]

*shrug* PL_strdup wouldn't copy embedded nulls.

Comment 6

[Josh Aas]

Comment on attachment 378262 [details] [diff] [review]
patch

Thanks!

Comment 7

[Josh Aas]

Timothy - do you need someone to push this for you?

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to comment #7)
> Timothy - do you need someone to push this for you?

Yes I do. I was just going to add checkin-needed, unless you're volunteering?

Comment 9

[Josh Aas]

Comment on attachment 378262 [details] [diff] [review]
patch

We should take this for 1.9.1.

Comment 10

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/a3b152e3e972

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 378262 [details] [diff] [review]
patch

a191=beltzner

Comment 12

[Josh Aas]

pushed to mozilla-1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/90215ba771f8

I forgot to push the change as Timothy Nikkel on 1.9.1, he is credited correctly on trunk. Sorry!