nsPersistentProperties doesn't handle multi-line properties files correctly

RESOLVED FIXED

Status

()

Core
XPCOM
--
major
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: Philip K. Warren, Assigned: Philip K. Warren)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

1.08 KB, text/plain
Details
1.51 KB, patch
Darin Fisher
: review+
Alec Flett
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
Consider the following entry in a .properties file:

entry=This is the first line.\n\
\n\
This is the third line.

Java handles these .properties files properly. The result of parsing this file
in Java is the following:

entry=This is the first line.

This is the third line.

Mozilla handles these files incorrectly:

entry=This is the first line.
\nThis is the third line.

From the following in nsPersistentProperties.cpp, you can see where the error lies:

184            if (c == '\\') {
185              c = Read();
186              switch(c) {
187                case '\r':
188                case '\n':
189                  c = SkipWhiteSpace(c);
190                  value.Append((PRUnichar) c);
191                  break;

This will read until the first '\', which occurs at the end of a line, so this
is a multiline properties entry. Then it encounters the end of the first line,
and skips until the next non-whitespace character, which is another '\'. It then
appends the literal '\' character, and continues on until the 'n'. It then
appends the 'n' character, and so we get the literal string '\n' appended
instead of a newline.
(Assignee)

Comment 1

14 years ago
Created attachment 131620 [details]
Java test program

This is the Java test program I used to test.
(Assignee)

Comment 2

14 years ago
Created attachment 131621 [details] [diff] [review]
Patch v1

Patch which fixes the problem.
(Assignee)

Updated

14 years ago
Attachment #131621 - Flags: superreview?(alecf)
Attachment #131621 - Flags: review?(darin)

Comment 3

14 years ago
Comment on attachment 131621 [details] [diff] [review]
Patch v1

looks good, but do LOTS of testing, and be prepared to back out - we've had
problems with this code before, and it affects a lot of people - I doubt that
many folks are using multi-line .properties files in ENGLISH, but in other
languages they may... 

i.e. make sure to test with cases beyond just \n

i.e. test to make sure

foo=alpha\
beta\
gamma

and make sure you get alphabetagamma. Also make sure that special characters
like 'u' and 'n' don't trigger something wierd, like

foo=I did it all for the \
nookie
Attachment #131621 - Flags: superreview?(alecf) → superreview+
(Assignee)

Comment 4

14 years ago
Taking ownership of bug.
Assignee: dougt → pkw
(Assignee)

Comment 5

14 years ago
We actually found this while testing Mozilla in other languages. The tools our
translators are using expected .properties files to behave similarly to Java
.properties files, so this may be why this was not caught before.

I have tested the examples you have shown and they are working correctly. I have
come up with one more incompatibility between the Java .properties parser and
the Mozilla .properties parser (both before and after this patch):

foo=alpha\
beta\
gamma\
 <-- Single space here
 <-- Single space here
foo2=I did it all for the \
nookie

With Java's parser, you get the following:

foo=alphabetagamma
foo2=I did it all for the nookie

With Mozilla's parser, you get:

Key:   foo
Value: alphabetagammafoo2=I did it all for the nookie
Status: NEW → ASSIGNED

Comment 6

14 years ago
thanks for being thorough - I got bit by quirks in this file about 6 months ago...

Comment 7

14 years ago
Comment on attachment 131621 [details] [diff] [review]
Patch v1

r=darin
Attachment #131621 - Flags: review?(darin) → review+
(Assignee)

Comment 8

14 years ago
Created attachment 131774 [details] [diff] [review]
Patch v2

Better patch. Fixes other inconsistency noted with Mozilla's .properties file
parser vs. Java parser. Removes some redundant checks in SkipLine and
SkipWhiteSpace.
(Assignee)

Updated

14 years ago
Attachment #131621 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #131774 - Flags: superreview?(alecf)
Attachment #131774 - Flags: review?(darin)

Comment 9

14 years ago
Comment on attachment 131774 [details] [diff] [review]
Patch v2

hmm.. while I'm sure these are the right changes, how do you feel about landing
this stuff in stages? I know it seems like you're not changing much, but I've
been bitten by some wierd uses of .properties files - people rely on wierd
quirks of parsing, etc! I spent a good hour testing some changes I had made
before landing changes like this, and it took two weeks and 3 fixer-upper
patches before everything settled down again.. :(

why don't you land the other patch, wait a day, and land this one. that will
make it much easier for QA folks to determine which build (and thus which
patch) causes a problem... sr=alecf on the changes though
Attachment #131774 - Flags: superreview?(alecf) → superreview+
(Assignee)

Comment 10

14 years ago
I have checked in the first patch (as alecf has suggested) to ensure there are
no major regressions.

Checking in nsPersistentProperties.cpp;
                                      
/cvsroot/mozilla/xpcom/ds/nsPersistentProperties.cpp,v  <-- 
nsPersistentProperties.cpp
new revision: 1.43; previous revision: 1.42
done

Comment 11

14 years ago
Comment on attachment 131774 [details] [diff] [review]
Patch v2

>Index: xpcom/ds/nsPersistentProperties.cpp

> PRInt32
> nsPersistentProperties::SkipWhiteSpace(PRInt32 c)
> {
>-  while ((c >= 0) && IS_WHITE_SPACE(c)) {
>+  while (IS_WHITE_SPACE(c)) {
>     c = Read();
>   }

here, (c >= 0) is redundant, and it clearly makes sense to remove it.


> PRInt32
> nsPersistentProperties::SkipLine(PRInt32 c)
> {
>-  while ((c >= 0) && (c != '\r') && (c != '\n')) {
>+  while (c != '\r' && c != '\n') {
>     c = Read();
>   }

however, here... by removing (c >= 0) you are now being more accepting.
i do not know enough about the context of this function to be able to
say off hand whether or not that matters, but it does seem suspect to
me.

convince me that you can remove this (c >= 0) check or put it back,
and r=me ;-)
(Assignee)

Comment 12

14 years ago
Darin - you are correct, this was a careless mistake. I will check in the second
patch without this change. Thanks.

Comment 13

14 years ago
Comment on attachment 131774 [details] [diff] [review]
Patch v2

r=darin with that change
Attachment #131774 - Flags: review?(darin) → review+
(Assignee)

Comment 14

14 years ago
Checked in second patch.

Checking in nsPersistentProperties.cpp;
                                      
/cvsroot/mozilla/xpcom/ds/nsPersistentProperties.cpp,v  <-- 
nsPersistentProperties.cpp
new revision: 1.44; previous revision: 1.43
done
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.