Bugzilla@Mozilla – Bug 397093
Faulty .properties file results in uninitialized memory being used
Last modified: 2009-01-05 11:57:28 PST
Summon comment box
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.
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"));
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?
(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.
(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.
OK, I can reproduce the problem in a branch build (I was testing trunk earlier).
Created attachment 282232 [details] screenshot
Thanks for raising the visibility on this one, Jesse, but probably not enough time to fix for the upcoming 2.0.0.12 release.
Created attachment 306514 [details] [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 on attachment 306514 [details] [review] Branch patch I'm not going to review this as a branch patch until an equivalent trunk patch is here with unit tests.
Created attachment 307421 [details] [review] Trunk patch
Comment on attachment 306514 [details] [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.
Moving to the next release since this doesn't yet have reviews.
Comment on attachment 306514 [details] [review] Branch patch Still want a test for this. Even a binary test in xpcom/tests would help me feel comfortable.
Comment on attachment 307421 [details] [review] Trunk patch Requesting approval for the trunk patch.
Comment on attachment 307421 [details] [review] Trunk patch Let's get a test per bsmedberg's suggestion. Re-request approval once tests are included.
I could be wrong, but I think bsmedberg was requesting a test for the branch patch. The trunk patch has a test already...
Comment on attachment 307421 [details] [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)
Created attachment 308810 [details] [review] Branch patch with test added to StringBundleTest.cpp
Comment on attachment 307421 [details] [review] Trunk patch a1.9+=damons
Comment on attachment 308810 [details] [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 on attachment 308810 [details] [review] Branch patch with test added to StringBundleTest.cpp Approved for 1.8.1.14. a=ss for release-drivers.
Trunk and branch patches are both checked in.
http://mxr.mozilla.org/mozilla/source/intl/strres/tests/unit/test_bug397093.js
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.
(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 on attachment 308810 [details] [review] Branch patch with test added to StringBundleTest.cpp a=asac for 1.8.0