397093 - Faulty .properties file results in uninitialized memory being used

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Daniel Glazman (:glazou) (NOT MONITORING BMO ANY MORE)]

Attachment: faulty properties file, bad encoding

I have hit a weird bug writing an extension for Firefox. The extension is localized in both french and english.
One of the french *.properties file was not UTF-8 but ISO8859-encoded.
The window opened by my extension retrieves 3 strings from that properties file.
Surprisingly, the first retrieval (string 'textLayouts' in the attached file)
did not fail, while the two others did fail.
BUT what I got back from that first |GetStringFromName()| call was absolutely not
what's in the properties file but some text coming from a MS Word process also
running on my machine !!! My XUL window was showing me a bit of the text I was
editing in Word...
Fixing the encoding of the file of course resolved the issue.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Could you provide a minimal XUL testcase that shows the problem (when run with privileges)? Running the following code snippet gives me just "Mon":

var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]. getService(Components.interfaces.nsIStringBundleService); var sb = sbs.createBundle("file:///C:/Users/Gavin/Desktop/layout2.properties"); prompt('',sb.GetStringFromName("textLayouts"));

Comment 2

[:Gavin Sharp [email: [e-mail]]]

I also don't understand how it could have shown you data from another process - Windows' memory protection should prevent that from happening, and even if it didn't, the odds of our code having read memory from Word in such a way that it then displayed text from the opened Word file is rather low. Are you sure it wasn't a fluke of some sort? Can you reproduce the problem?

Comment 3

[Daniel Glazman (:glazou) (NOT MONITORING BMO ANY MORE)]

(In reply to comment #1)
> Could you provide a minimal XUL testcase that shows the problem (when run with
> privileges)? Running the following code snippet gives me just "Mon":
> 
> var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"].
> getService(Components.interfaces.nsIStringBundleService); var sb =
> sbs.createBundle("file:///C:/Users/Gavin/Desktop/layout2.properties");
> prompt('',sb.GetStringFromName("textLayouts"));

My code is exactly similar to yours. But I got back a string starting with that
"Mod" (and not "Mon") followed immediately by text coming from the other
process.
I am running 2.0.0.7 on winXP sp2.

Comment 4

[Daniel Glazman (:glazou) (NOT MONITORING BMO ANY MORE)]

(In reply to comment #2)

> Can you reproduce the problem?

Partly yes. I am not this time seeing text coming from another process
but from the MIT license plate of one of my JS files. That license text is of
course contained in a JS comment... Just to be clear, that JS file is not the
one retrieving the localized strings.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

OK, I can reproduce the problem in a branch build (I was testing trunk earlier). 

Comment 6

[Daniel Glazman (:glazou) (NOT MONITORING BMO ANY MORE)]

Attachment: screenshot

Comment 7

[Daniel Veditz [:dveditz]]

Thanks for raising the visibility on this one, Jesse, but probably not enough time to fix for the upcoming 2.0.0.12 release.

Comment 8

[Simon Montagu :smontagu]

Attachment: Branch patch

This is a deliberately minimal patch for branch so that no property in a property file which currently works will stop working.

For trunk I would rather return an error and fail to load the entire property file if it's not valid UTF-8.

Comment 9

[Benjamin Smedberg]

Comment on attachment 306514 [details] [diff] [review]
Branch patch

I'm not going to review this as a branch patch until an equivalent trunk patch is here with unit tests.

Comment 10

[Simon Montagu :smontagu]

Attachment: Trunk patch

Comment 11

[Simon Montagu :smontagu]

Comment on attachment 306514 [details] [diff] [review]
Branch patch

I already said in comment 8 that the trunk patch wasn't going to be equivalent. The patch I just attached for trunk rejects the whole properties file if it encounters invalid UTF-8, but on branch I think we should preserve the status quo that properties earlier in the file without encoding errors can still be retrieved.

Comment 12

[Samuel Sidler (old account; do not CC)]

Moving to the next release since this doesn't yet have reviews.

Comment 13

[Benjamin Smedberg]

Comment on attachment 306514 [details] [diff] [review]
Branch patch

Still want a test for this. Even a binary test in xpcom/tests would help me feel comfortable.

Comment 14

[Simon Montagu :smontagu]

Comment on attachment 307421 [details] [diff] [review]
Trunk patch

Requesting approval for the trunk patch.

Comment 15

[Damon Sicore (:damons)]

Comment on attachment 307421 [details] [diff] [review]
Trunk patch

Let's get a test per bsmedberg's suggestion.  Re-request approval once tests are included.

Comment 16

[Samuel Sidler (old account; do not CC)]

I could be wrong, but I think bsmedberg was requesting a test for the branch patch. The trunk patch has a test already...

Comment 17

[Simon Montagu :smontagu]

Comment on attachment 307421 [details] [diff] [review]
Trunk patch

Yes, what Samuel said. This is the trunk patch, with a test, which Ben already gave r+sr to. (This should really be blocking 1.9+ too)

Comment 18

[Simon Montagu :smontagu]

Attachment: Branch patch with test added to StringBundleTest.cpp

Comment 19

[Damon Sicore (:damons)]

Comment on attachment 307421 [details] [diff] [review]
Trunk patch

a1.9+=damons

Comment 20

[Simon Montagu :smontagu]

Comment on attachment 308810 [details] [diff] [review]
Branch patch with test added to StringBundleTest.cpp

Very safe fix ensuring that the length returned by UTF8InputStream::Read doesn't exceed the number of characters successfully converted.

Comment 21

[Samuel Sidler (old account; do not CC)]

Comment on attachment 308810 [details] [diff] [review]
Branch patch with test added to StringBundleTest.cpp

Approved for 1.8.1.14. a=ss for release-drivers.

Comment 22

[Simon Montagu :smontagu]

Trunk and branch patches are both checked in.

Comment 23

[Jeff Walden [:Waldo]]

http://mxr.mozilla.org/mozilla/source/intl/strres/tests/unit/test_bug397093.js

Comment 24

[Daniel Veditz [:dveditz]]

Why is this sg:moderate? text from .properties files isn't normally accessible to web content in any way even if there is such a malformed file. We also don't know of any .properties files shipped with Firefox that had this problem.

Comment 25

[Daniel Glazman (:glazou) (NOT MONITORING BMO ANY MORE)]

(In reply to comment #24)
> Why is this sg:moderate? text from .properties files isn't normally accessible
> to web content in any way even if there is such a malformed file. We also don't
> know of any .properties files shipped with Firefox that had this problem.

Extensions ?

Comment 26

[Alexander Sack]

Comment on attachment 308810 [details] [diff] [review]
Branch patch with test added to StringBundleTest.cpp

a=asac for 1.8.0