Closed Bug 397093 Opened 17 years ago Closed 17 years ago

Faulty .properties file results in uninitialized memory being used

Categories

(Core :: Internationalization, defect)

1.8 Branch
x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: glazou, Assigned: smontagu)

Details

(Keywords: fixed1.8.1.15, Whiteboard: [sg:low] 1.8 branch, requires faulty localization/addon)

Attachments

(4 files, 1 obsolete file)

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).
Assignee: dveditz → smontagu
Component: Security → Internationalization
Flags: blocking1.8.1.12?
QA Contact: toolkit → i18n
Summary: Firefox showing data from other win process → Faulty .properties file results in data from other parts of memory being used
Whiteboard: [sg:moderate] 1.8 branch
Thanks for raising the visibility on this one, Jesse, but probably not enough time to fix for the upcoming 2.0.0.12 release.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.13?
Flags: blocking1.8.1.12?
Flags: blocking1.8.1.13? → blocking1.8.1.13+
Attached patch Branch patch (obsolete) — Splinter Review
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.
Attachment #306514 - Flags: superreview?(benjamin)
Attachment #306514 - Flags: review?(benjamin)
Whiteboard: [sg:moderate] 1.8 branch → [sg:moderate] 1.8 branch, need r=bsmedberg
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.
Attachment #306514 - Flags: superreview?(benjamin)
Attachment #306514 - Flags: review?(benjamin)
Summary: Faulty .properties file results in data from other parts of memory being used → Faulty .properties file results in uninitialized memory being used
Whiteboard: [sg:moderate] 1.8 branch, need r=bsmedberg → [sg:moderate] 1.8 branch, requires faulty localization/addon, need r=bsmedberg
Attached patch Trunk patchSplinter Review
Attachment #307421 - Flags: superreview?(benjamin)
Attachment #307421 - Flags: review?(benjamin)
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.
Attachment #306514 - Flags: superreview?(benjamin)
Attachment #306514 - Flags: review?(benjamin)
Moving to the next release since this doesn't yet have reviews.
Flags: blocking1.8.1.14+
Flags: blocking1.8.1.13-
Flags: blocking1.8.1.13+
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.
Attachment #306514 - Flags: superreview?(benjamin)
Attachment #306514 - Flags: review?(benjamin)
Attachment #307421 - Flags: superreview?(benjamin)
Attachment #307421 - Flags: superreview+
Attachment #307421 - Flags: review?(benjamin)
Attachment #307421 - Flags: review+
Comment on attachment 307421 [details] [diff] [review] Trunk patch Requesting approval for the trunk patch.
Attachment #307421 - Flags: approval1.9?
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.
Attachment #307421 - Flags: approval1.9?
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] [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)
Attachment #307421 - Flags: approval1.9?
Attachment #306514 - Attachment is obsolete: true
Attachment #308810 - Flags: superreview?(benjamin)
Attachment #308810 - Flags: review?(benjamin)
Comment on attachment 307421 [details] [diff] [review] Trunk patch a1.9+=damons
Attachment #307421 - Flags: approval1.9? → approval1.9+
Attachment #308810 - Flags: superreview?(benjamin)
Attachment #308810 - Flags: superreview+
Attachment #308810 - Flags: review?(benjamin)
Attachment #308810 - Flags: review+
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.
Attachment #308810 - Flags: approval1.8.1.14?
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.
Attachment #308810 - Flags: approval1.8.1.14? → approval1.8.1.14+
Trunk and branch patches are both checked in.
Status: NEW → RESOLVED
Closed: 17 years ago
Keywords: fixed1.8.1.14
Resolution: --- → FIXED
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.
Whiteboard: [sg:moderate] 1.8 branch, requires faulty localization/addon, need r=bsmedberg → [sg:low] 1.8 branch, requires faulty localization/addon
Group: security
(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 ?
Attachment #308810 - Flags: approval1.8.0.next+
Comment on attachment 308810 [details] [diff] [review] Branch patch with test added to StringBundleTest.cpp a=asac for 1.8.0
Flags: blocking1.8.0.next+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: