Closed
Bug 397093
Opened 17 years ago
Closed 17 years ago
Faulty .properties file results in uninitialized memory being used
Categories
(Core :: Internationalization, defect)
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)
309 bytes,
text/plain
|
Details | |
2.93 KB,
image/png
|
Details | |
6.40 KB,
patch
|
benjamin
:
review+
benjamin
:
superreview+
damons
:
approval1.9+
|
Details | Diff | Splinter Review |
3.90 KB,
patch
|
benjamin
:
review+
benjamin
:
superreview+
samuel.sidler+old
:
approval1.8.1.15+
asac
:
approval1.8.0.next+
|
Details | Diff | Splinter Review |
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•17 years ago
|
||
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•17 years ago
|
||
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?
Reporter | ||
Comment 3•17 years ago
|
||
(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.
Reporter | ||
Comment 4•17 years ago
|
||
(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•17 years ago
|
||
OK, I can reproduce the problem in a branch build (I was testing trunk earlier).
Reporter | ||
Comment 6•17 years ago
|
||
Updated•17 years ago
|
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
Comment 7•17 years ago
|
||
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?
Updated•17 years ago
|
Flags: blocking1.8.1.13? → blocking1.8.1.13+
Assignee | ||
Comment 8•17 years ago
|
||
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)
Updated•17 years ago
|
Whiteboard: [sg:moderate] 1.8 branch → [sg:moderate] 1.8 branch, need r=bsmedberg
Comment 9•17 years ago
|
||
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)
Updated•17 years ago
|
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
Assignee | ||
Comment 10•17 years ago
|
||
Attachment #307421 -
Flags: superreview?(benjamin)
Attachment #307421 -
Flags: review?(benjamin)
Assignee | ||
Comment 11•17 years ago
|
||
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)
Comment 12•17 years ago
|
||
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 13•17 years ago
|
||
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)
Updated•17 years ago
|
Attachment #307421 -
Flags: superreview?(benjamin)
Attachment #307421 -
Flags: superreview+
Attachment #307421 -
Flags: review?(benjamin)
Attachment #307421 -
Flags: review+
Assignee | ||
Comment 14•17 years ago
|
||
Comment on attachment 307421 [details] [diff] [review]
Trunk patch
Requesting approval for the trunk patch.
Attachment #307421 -
Flags: approval1.9?
Comment 15•17 years ago
|
||
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?
Comment 16•17 years ago
|
||
I could be wrong, but I think bsmedberg was requesting a test for the branch patch. The trunk patch has a test already...
Assignee | ||
Comment 17•17 years ago
|
||
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?
Assignee | ||
Comment 18•17 years ago
|
||
Attachment #306514 -
Attachment is obsolete: true
Attachment #308810 -
Flags: superreview?(benjamin)
Attachment #308810 -
Flags: review?(benjamin)
Comment 19•17 years ago
|
||
Comment on attachment 307421 [details] [diff] [review]
Trunk patch
a1.9+=damons
Attachment #307421 -
Flags: approval1.9? → approval1.9+
Updated•17 years ago
|
Attachment #308810 -
Flags: superreview?(benjamin)
Attachment #308810 -
Flags: superreview+
Attachment #308810 -
Flags: review?(benjamin)
Attachment #308810 -
Flags: review+
Assignee | ||
Comment 20•17 years ago
|
||
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 21•17 years ago
|
||
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+
Assignee | ||
Comment 22•17 years ago
|
||
Trunk and branch patches are both checked in.
Comment 23•17 years ago
|
||
Flags: in-testsuite+
Comment 24•17 years ago
|
||
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
Updated•17 years ago
|
Group: security
Reporter | ||
Comment 25•17 years ago
|
||
(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 ?
Updated•16 years ago
|
Attachment #308810 -
Flags: approval1.8.0.next+
Comment 26•16 years ago
|
||
Comment on attachment 308810 [details] [diff] [review]
Branch patch with test added to StringBundleTest.cpp
a=asac for 1.8.0
Updated•16 years ago
|
Flags: blocking1.8.0.next+
You need to log in
before you can comment on or make changes to this bug.
Description
•