Last Comment Bug 397093 - Faulty .properties file results in uninitialized memory being used
: Faulty .properties file results in uninitialized memory being used
Status: RESOLVED FIXED
[sg:low] 1.8 branch, requires faulty ...
: fixed1.8.1.15
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: 1.8 Branch
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Simon Montagu :smontagu
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-09-21 12:34 PDT by Daniel Glazman (:glazou)
Modified: 2009-01-05 11:57 PST (History)
10 users (show)
samuel.sidler+old: blocking1.8.1.13-
samuel.sidler+old: blocking1.8.1.15+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
jwalden+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
faulty properties file, bad encoding (309 bytes, text/plain)
2007-09-21 12:34 PDT, Daniel Glazman (:glazou)
no flags Details
screenshot (2.93 KB, image/png)
2007-09-25 02:29 PDT, Daniel Glazman (:glazou)
no flags Details
Branch patch (1.29 KB, patch)
2008-02-29 06:44 PST, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Trunk patch (6.40 KB, patch)
2008-03-05 00:04 PST, Simon Montagu :smontagu
benjamin: review+
benjamin: superreview+
dsicore: approval1.9+
Details | Diff | Splinter Review
Branch patch with test added to StringBundleTest.cpp (3.90 KB, patch)
2008-03-11 23:54 PDT, Simon Montagu :smontagu
benjamin: review+
benjamin: superreview+
samuel.sidler+old: approval1.8.1.15+
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Description Daniel Glazman (:glazou) 2007-09-21 12:34:56 PDT
Created attachment 281857 [details]
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: gavin@gavinsharp.com] 2007-09-21 12:58:16 PDT
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: gavin@gavinsharp.com] 2007-09-21 13:01:54 PDT
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) 2007-09-21 13:04:05 PDT
(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) 2007-09-21 13:10:07 PDT
(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: gavin@gavinsharp.com] 2007-09-21 13:10:56 PDT
OK, I can reproduce the problem in a branch build (I was testing trunk earlier). 
Comment 6 Daniel Glazman (:glazou) 2007-09-25 02:29:09 PDT
Created attachment 282232 [details]
screenshot
Comment 7 Daniel Veditz [:dveditz] 2008-01-16 13:36:26 PST
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 2008-02-29 06:44:45 PST
Created attachment 306514 [details] [diff] [review]
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 [:bsmedberg] 2008-03-04 17:50:24 PST
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 2008-03-05 00:04:32 PST
Created attachment 307421 [details] [diff] [review]
Trunk patch
Comment 11 Simon Montagu :smontagu 2008-03-05 00:09:35 PST
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) 2008-03-07 12:02:13 PST
Moving to the next release since this doesn't yet have reviews.
Comment 13 Benjamin Smedberg [:bsmedberg] 2008-03-11 06:52:20 PDT
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 2008-03-11 09:24:58 PDT
Comment on attachment 307421 [details] [diff] [review]
Trunk patch

Requesting approval for the trunk patch.
Comment 15 Damon Sicore (:damons) 2008-03-11 17:48:40 PDT
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) 2008-03-11 19:59:20 PDT
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 2008-03-11 21:27:26 PDT
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 2008-03-11 23:54:41 PDT
Created attachment 308810 [details] [diff] [review]
Branch patch with test added to StringBundleTest.cpp
Comment 19 Damon Sicore (:damons) 2008-03-12 14:21:13 PDT
Comment on attachment 307421 [details] [diff] [review]
Trunk patch

a1.9+=damons
Comment 20 Simon Montagu :smontagu 2008-03-13 08:48:32 PDT
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) 2008-03-26 11:14:03 PDT
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 2008-03-28 00:32:12 PDT
Trunk and branch patches are both checked in.
Comment 23 Jeff Walden [:Waldo] (remove +bmo to email) 2008-04-03 20:57:23 PDT
http://mxr.mozilla.org/mozilla/source/intl/strres/tests/unit/test_bug397093.js
Comment 24 Daniel Veditz [:dveditz] 2008-07-02 01:09:37 PDT
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) 2008-07-02 02:00:35 PDT
(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 2009-01-05 11:57:07 PST
Comment on attachment 308810 [details] [diff] [review]
Branch patch with test added to StringBundleTest.cpp

a=asac for 1.8.0

Note You need to log in before you can comment on or make changes to this bug.