288400 - stringbundle can crash if given bad input

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P5)

Description

[timeless]

js>
bdl=[];bdl[100]='';bdl=Components.classes["@mozilla.org/intl/stringbundle;1"].getService(Components.interfaces.nsIStringBundleService).createBundle("data:text/plain,a=hi"+bdl.join("
%s")); bdl.formatStringFromName("a",[],0)
###!!! ASSERTION: FormatStringFromName() without format parameters: use
GetStringFromName() instead:
 'aParams && aLength', file r:/mozilla/intl/strres/src/nsStringBundle.cpp, line 233
###!!! ASSERTION: not a UTF8 string: 'Error', file
../../dist/include/string\nsUTF8Utils.h, line 151

###!!! ASSERTION: Input wasn't UTF8 or incorrect length was calculated: 'Error',
file r:/mozilla/xpcom/string/src/nsReadableUtils.cpp, line 262
###!!! ASSERTION: Not a UTF-8 string. This code should only be used for
converting from known UTF-8 strings.: 'Error', file
../../../dist/include/string\nsUTF8Utils.h, line 264

Unhandled exception at 0x102157ac (msvcr71d.dll) in xpcshell.exe: 0xC0000005:
Access violation reading location 0x00000011.

>	msvcr71d.dll!strlen()  Line 66	Asm
 	xpcom_core.dll!nsCharTraits<char>::length(const char * s=0x00000011)  Line
552 + 0x9	C++
 	xpcom_core.dll!nsDependentCString::nsDependentCString(const char *
data=0x00000011)  Line 90 + 0x12	C++
 	xpcom_core.dll!AppendUTF8toUTF16(const char * aSource=0x00000011, nsAString &
aDest={...})  Line 293 + 0x10	C++
 	xpcom_core.dll!NS_ConvertUTF8toUTF16::NS_ConvertUTF8toUTF16(const char *
aCString=0x00000011)  Line 173 + 0xd	C++
 	xpcom_core.dll!cvt_s(SprintfStateStr * ss=0x0012dcf0, const char *
s=0x00000011, int width=0, int prec=-1, int flags=0)  Line 560	C++
 	xpcom_core.dll!dosprintf(SprintfStateStr * ss=0x0012dcf0, const unsigned short
* fmt=0x01573c24, char * ap=0x0012dd9c)  Line 1138 + 0x1c	C++
 	xpcom_core.dll!nsTextFormatter::vsmprintf(const unsigned short *
fmt=0x01573b78, char * ap=0x0012dd2c)  Line 1292 + 0x11	C++
 	xpcom_core.dll!nsTextFormatter::smprintf(const unsigned short *
fmt=0x01573b78, ...)  Line 1245 + 0xd	C++
 	i18n.dll!nsStringBundle::FormatString(const unsigned short *
aFormatStr=0x01573b78, const unsigned short * * aParams=0x00000000, unsigned int
aLength=0, unsigned short * * aResult=0x0012dfdc)  Line 400 + 0x121	C++
 	i18n.dll!nsStringBundle::FormatStringFromName(const unsigned short *
aName=0x00c1fff8, const unsigned short * * aParams=0x00000000, unsigned int
aLength=0, unsigned short * * aResult=0x0012dfdc)  Line 244 + 0x1d	C++
 	xpcom_core.dll!XPTC_InvokeByIndex(nsISupports * that=0x00c19ca0, unsigned int
methodIndex=6, unsigned int paramCount=4, nsXPTCVariant * params=0x0012dfac) 
Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...},
XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2068 + 0x1e	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x00c14e08, JSObject *
obj=0x015968c8, unsigned int argc=3, long * argv=0x00c1603c, long *
vp=0x0012e280)  Line 1311 + 0xb	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x00c14e08, unsigned int argc=3, unsigned
int flags=0)  Line 1293 + 0x20	C
 	js3250.dll!js_Interpret(JSContext * cx=0x00c14e08, unsigned char *
pc=0x00c1f8f1, long * result=0x0012ed1c)  Line 3566 + 0xf	C
 	js3250.dll!js_Execute(JSContext * cx=0x00c14e08, JSObject * chain=0x00b393e0,
JSScript * script=0x00c1f860, JSStackFrame * down=0x00000000, unsigned int
flags=0, long * result=0x0012fdcc)  Line 1523 + 0x13	C
 	js3250.dll!JS_ExecuteScript(JSContext * cx=0x00c14e08, JSObject *
obj=0x00b393e0, JSScript * script=0x00c1f860, long * rval=0x0012fdcc)  Line
3657 + 0x19	C
 	xpcshell.exe!ProcessFile(JSContext * cx=0x00c14e08, JSObject * obj=0x00b393e0,
const char * filename=0x00000000, _iobuf * file=0x1027c838)  Line 646 + 0x16	C++
 	xpcshell.exe!Process(JSContext * cx=0x00c14e08, JSObject * obj=0x00b393e0,
const char * filename=0x00000000)  Line 699 + 0x15	C++
 	xpcshell.exe!ProcessArgs(JSContext * cx=0x00c14e08, JSObject * obj=0x00b393e0,
char * * argv=0x0036706c, int argc=0)  Line 827 + 0x11	C++
 	xpcshell.exe!main(int argc=0, char * * argv=0x0036706c, char * *
envp=0x00362f98)  Line 1639 + 0x15	C++
 	xpcshell.exe!mainCRTStartup()  Line 398 + 0x11	C

i found this crash in 2001, but it seems i never got around to filing it :(

Comment 1

[Simon Montagu :smontagu]

Attachment: patch

I get a feeling of deja vu... see bug 268832 and bug 269893. With this patch I
see:

bdl=[];bdl[100]='';bdl=Components.classes["@mozilla.org/intl/stringbundle;1"].getService(Components.interfaces.nsIStringBundleService).createBundle("data:text/plain,a=hi"+bdl.join("%s"));
bdl.formatStringFromName("a",[],0)
hi%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s


Is that as expected?

Comment 2

[[e-mail]]

Comment on attachment 179146 [details] [diff] [review]
patch

bdl.formatStringFromName("a",[""],1) will still crash...

Comment 3

[timeless]

yeah, i was planning to reinvent parsing, possibly using a js component.

Comment 4

[timeless]

Attachment: replacement

So. I've been meaning to do this for years. Here is a rough draft.

I believe it's bug for bug compatible with the existing behavior (ignoring crashes), tested w/ the code sections from
* this bug's testcase
* nsSidebar.js
* coverage-neterror.js

Note that this version doesn't have a 10 argument limit. 

> bdl=[];bdl[100]='';q=[];for(i=0;i<bdl.length;++i)q[i]=5000-Math.pow(i,2);bdl=s.createBundle("data:text/plain,a=> hi"+bdl.join(" %s"));bdl.formatStringFromName("a",q,q.length)
> hi 5000 4999 4996 4991 4984 4975 4964 4951 4936 4919 4900 4879 4856 4831 4804 4775 4744 4711 4676 4639 4600 4559 4516 4471 4424 4375 4324 4271 4216 4159 4100 4039 3976 3911 3844 3775 3704 3631 3556 3479 3400 3319 3236 3151 3064 2975 2884 2791 2696 2599 2500 2399 2296 2191 2084 1975 1864 1751 1636 1519 1400 1279 1156 1031 904 775 644 511 376 239 100 -41 -184 -329 -476 -625 -776 -929 -1084 -1241 -1400 -1561 -1724 -1889 -2056 -2225 -2396 -2569 -2744 -2921 -3100 -3281 -3464 -3649 -3836 -4025 -4216 -4409 -4604 -4801

Comment 5

[Robert Kaiser]

...and I so much hoped we would be able to obsolete the string bundle file format as well as the DTD-L10n stuff by moving to L20n before anyone would need to put actual work into any code supporting stringbundles or so. Unfortunately, nothing has been moving on that front for years now :(

How do XUL <stringbundle>s act there? do we need to fix those as well in some way?

Comment 6

[timeless]

that shouldn't matter, it's a theoretical drop in replacement for a lower level thing. I'm just tired of stringbundles crashing, and it wasn't that much work to convert. Note that converting to JS is *not* a commitment to supporting the contract further, just a way to get rid of the c++ (there's another crash bug i poked a few weeks ago which is mostly why i decided to work on this).

Comment 7

[timeless]

Attachment: support extensible bundle

oops, I somehow neglected to look for c++ impls even though i checked for them in older versions :o.

this version survives the testcase from bug 485511.

To make testing easier, i've now included a testService() function so that you can do this:

xpcshell -f components/nsStringBundleService.js -f -
js> testService()
js> sbs=Components.classes[NS_STRINGBUNDLE_CONTRACTID].getService(Components.interfaces.nsIStringBundleService);c="content-sniffing-services";d=sbs.createExtensibleBundle(c);d.formatStringFromName("stupid",["a"],0)
uncaught exception: [Exception... "'Failure' when calling method: [nsIStringBundle::formatStringFromName]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location:
 "JS frame :: typein :: <TOP_LEVEL> :: line 5"  data: no]

Comment 8

[timeless]

Attachment: updated

ok, this component seems to work, note that you must apply the patch from bug 486311 because of a bug in caps...

Comment 9

[Robert Kaiser]

Just one question, how does this implementation work if %1$S isn't part of the string and %2$S is (just because we came across this case in L10n talk around PluralForms with stringbundles as the .replace stuff we're doing in many cases is lame when we actually have .getFormattedString)

Comment 10

[timeless]

> bdl=Components.classes["@mozilla.org/intl/stringbundle;1"].getService(Components.interfaces.nsIStringBundleService).createBundle("data:text/plain,a=%2$S").formatStringFromName("a",["hello","world"],2) 
world

Which seems perfectly reasonable.
The code doesn't care about much, however neither it nor i make promises to anyone foolish enough to mix %s and %1$s

> bdl=Components.classes["@mozilla.org/intl/stringbundle;1"].getService(Components.interfaces.nsIStringBundleService).createBundle("data:text/plain,a=%s %1$s %s").formatStringFromName("a",["hello","world"],2)
hello hello world

I'm currently working on nsStringBundleTextOverride so that we can retire the directory....

Comment 11

[timeless]

Attachment: support override service

ok, with this, all that's left is deleting the c++ directory and the references in build

Comment 12

[Phil Ringnalda (:philor)]

There may be other uses of other odd formats that dosprintf supports and you don't, but the one I spotted quickly scanning the first 1000 instances of "%" in .properties is XMLParsingError's use of %3$u.

Comment 13

[Phil Ringnalda (:philor)]

... which doesn't actually matter since it rolls its own formatting.

Comment 14

[timeless]

Attachment: support %%, %c, %x, %X, and %.2s

I'm willing to add it if someone wants it, note that there's no distinction between %d and %u since the formatter is passed strings not numbers.

%% is actually used
%c is used by mail nsImapMailFolder::Rename (for 321 below)

%lu seems to skip the stringbundle formatter, see nsImapProtocol::ShowProgress
%ld seems to skip the stringbundle formatter, see nsPop3Protocol::ProcessProtocolState

I claim that %l... isn't useful iven the stringbundle APIs, the only big question I have is whether anyone expects <commas> from their numbers.

/mail/locales/en-US/chrome/messenger/imapMsgs.properties
    * line 190 -- 5036=%S Downloading message header %lu of %lu
    * line 321 -- 5065= The %c character is reserved on this imap server. Please choose another name. 
/mail/locales/en-US/chrome/messenger/localMsgs.properties
    * line 109 -- 4009=Received %ld of %ld messages
/mail/locales/en-US/chrome/messenger/messenger.properties
    * line 363 -- percent=%S%%
/mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties 
    * line 131 -- 12533=There was a problem including the file %.200s in the message. Would you like to continue saving the message without this file?

http://mxr-test.konigsberg.mozilla.org/comm-central/search?string=%3D.*[a-z]&regexp=on&find=properties%24&filter=%25

only has 1720 matches, I've bumped the limit on konigsberg (it now supports 5 digits dynamically.

speaking of stupid impls,
mailnews.messageid_browser.url=http://groups.google.com/groups?selm=%mid&rnum=1

sadly, it's a pref and it's a hand spun thing which uses %mid instead of %mid% (2007-08-04 bug 62033), so fixing it would require rewriting user prefs which is never fun.

Comment 15

[Axel Hecht [:Pike]]

%1$0.S is used in the Japanese localization to specify a zero-width string.

Comment 16

[timeless]

what does a zero width string mean?

Comment 17

[Axel Hecht [:Pike]]

"", literally. They remove the variable from display by making it zero width, so that "%1$0.Sfoo" renders as "foo".

Comment 18

[timeless]

oh, then my string width code is wrong, it's currently only doing padding, not truncating.

Are they using %1$.0S because the current code doesn't allow them to just omit it from the string?

I don't suppose someone knows what %.4x should do :)

Comment 19

[Axel Hecht [:Pike]]

(In reply to comment #18)
<...> 
> Are they using %1$.0S because the current code doesn't allow them to just omit
> it from the string?

Right.

> I don't suppose someone knows what %.4x should do :)

Whenever I'm asked about what that code should do, I start to reverse engineer from scratch.

The tests we have so far seem to not cover enough ground for this code, sadly.

Comment 20

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #19)
> [...]
> The tests we have so far seem to not cover enough ground for this code, sadly.

do you know where these tests live, Axel? I'd like to take a look at them and check to see what the existence of this patch does to them.

Comment 21

[Axel Hecht [:Pike]]

http://mxr.mozilla.org/mozilla-central/source/intl/strres/tests/unit/ has the only ones I know about.

I don't see any obvious candidates for the stringbundle xul element.

Comment 22

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #21)
> http://mxr.mozilla.org/mozilla-central/source/intl/strres/tests/unit/ has the
> only ones I know about.

ah right, silly me. :)

> I don't see any obvious candidates for the stringbundle xul element.

there's probably some opportunity for more test writing here.

Comment 23

[Rob Campbell [:rc] (:robcee)]

http://graphs-stage.mozilla.org/graph.html#show=544423,544413&sel=1238562078,1238767486

here are the two mac 10.4 try builders with Ts graphed out:

http://graphs-stage.mozilla.org/graph.html#show=544423,544413&sel=1238562078,1238767486

it looks like for the build with this patch, Ts shot up a good 10-20% over my guessed-at-median. It's a little hard to see decent numbers on the try servers because their numbers are all over the place. Startup time on Leopard servers are similar though starting higher depending on either machine speed or OS considerations (mac whiz-bang features).

On linux, I'm not sure it's quite as easy a read. Certainly looks higher over the lowest runs on these machines.

http://graphs-stage2.mozilla.org/graph.html#tests=[{%22test%22:%2212%22,%22branch%22:%226%22,%22machine%22:%22130%22}]

http://graphs-stage2.mozilla.org/graph.html#tests=[{%22test%22:%2212%22,%22branch%22:%226%22,%22machine%22:%22129%22}]

I'm not sure if there's a better way to link to these graphs or not. Probably. Might be best to just scroll to this date, April 3, 2009 at around 9:40 on Tinderbox and follow the links from there.

I feel confident in saying that there's going to be *some* penalty in browser startup time for this patch. Preventing a whole class of browser crashes (and the security benefits therein) may be reason enough to take it.

Comment 24

[Rob Campbell [:rc] (:robcee)]

I should also note that this was run against the penultimate copy of nsStringBundleService.js. I'll try to run the newer version during a quieter period this weekend.

Comment 25

[Simon Montagu :smontagu]

Comment on attachment 370826 [details] [diff] [review]
support %%, %c, %x, %X, and %.2s

I don't really feel competent to review this, either from the point of view of JS knowledge, or from the point of view of what consumers need. I can give moa when necessary, but I think it probably needs a higher-level architectural approval also (brendan?)

Comment 26

[timeless]

Attachment: support %5.2x

so.... the current nsIStringBundle.formatStringFromName is totally useless for %d and friends, which makes testing its behavior for things like %.1x useless.

js> s=Components.classes["@mozilla.org/intl/stringbundle;1"].getService(Components.interfaces.nsIStringBundleService);b=s.createBundle("data:text/plain,a=%d %lld %x %u");b.formatStringFromName("a",[1,2,3,4,5],5)
75681912 324945646459215824 437ab68 75654688
(note: these numbers are not constant, think about it if you like....)

and don't get me started with mixing formats, the code PR_ASSERT()s that it won't happen:

/* the fmt contains error Numbered Argument format, [e-mail] */
	PR_ASSERT(0);
	
Which means it's trivial for an accidental string bundle to cause the app to abort today. Although I guess that hasn't happened.

With my flavor %, things should be somewhat rational:

js> Components.
classes["@mozilla.org/intl/stringbundle;1"].
getService(Components.interfaces.nsIStringBundleService).
createBundle("data:text/plain,a=%1.0x;%1.1x;%2.1x;%.2x;%.0d;%1.0d;%1.1d;%2.1d;%.2d;%.0S;%5.s;%.5c;%.4s;").
formatStringFromName("a",[10,20,30,40,50,60,70,80,90,100,"help","cheap","world","blank"],14)
0x0;0x1;0x01;0x28;;0;7;08;90;; help;c;worl;

So, this covers pike's Japanese case - which is really another instance of the PR_ASSERT i mentioned above, gah. With my fix, they wouldn't need that for future versions....

And yes, that 14 is me showing off (see rather amusing comments in textformatter and the c++ stringbundle code).

Comment 27

[Rob Campbell [:rc] (:robcee)]

this morning's try build (with patch from c#26), passes unittests, but does exhibit some crashing on exit. not sure if that's automation.py not shutting down properly or what.

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1239021993.1239030062.12410.gz&fulltext=1

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1239021767.1239029702.11495.gz&fulltext=1

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1239018995.1239029702.11494.gz&fulltext=1

still waiting on talos runs. Note that these are using default prefs, i.e., no chrome jit.

Comment 28

[Rob Campbell [:rc] (:robcee)]

http://graphs-stage2.mozilla.org/graph.html#tests=[{%22test%22:%2212%22,%22branch%22:%226%22,%22machine%22:%22103%22}]&sel=1238568504,1239018960

Above link shows a Ts, hopefully in the range of April 1st to 6th. The two large peaks towards the end of the line are our test runs. They're looking like a full 10+% over the rough median just based on eyeball measurements.

http://graphs-stage2.mozilla.org/graph.html#tests=[{%22test%22:%2212%22,%22branch%22:%226%22,%22machine%22:%22130%22}]

Linux is better. So much so that I'm questioning if there's enough resolution in these graphs.

http://graphs-stage2.mozilla.org/graph.html#tests=[{%22test%22:%222%22,%22branch%22:%226%22,%22machine%22:%22103%22}]&sel=1238569636,1239018960

Tp is about 10% for today's run. Close to 20% for Friday's.

I can't really see anything on the linux talos box:

http://graphs-stage2.mozilla.org/graph.html#tests=[{%22test%22:%222%22,%22branch%22:%226%22,%22machine%22:%22130%22}]&sel=1238612835,1239018960

Comment 29

[Rob Campbell [:rc] (:robcee)]

windows results don't really look meaningful.

http://graphs-stage2.mozilla.org/graph.html#tests=[{%22test%22:12,%22branch%22:6,%22machine%22:102}]

Comment 30

[Axel Hecht [:Pike]]

Some general thoughts:

Creating a js copy of the code doesn't prevent consumers of the c++ api from crashing. They likely don't go the the XPCOM barrier on purpose to actually pass ints to the formatter. Which crashes happily if mistakingly interpreted via %s. Given that we probably continue to have the c++ code, I'm not sure if replacing it is a good idea.

I'm neither sure if the perf hit is OK. I don't understand our talos results enough, but chrome jit didn't fix that? Rob, did I read your comments right there? If so, we should get some help from there to see why, IMHO. There's additional interest here from my side, as my plans to introduce l20n include implementing it in JS, too, and I might be performing similar tasks as the current string bundle code. And I don't want to be slow ;-)

Comment 31

[timeless]

Attachment: JIT additions

I believe I can improve the performance by replacing the regexp w/ a for loop and allowing JIT to kick in.

replacing the c++ impl w/ the js impl will remove the few remaining crashers that we have, and result in better output for other cases and better input (your Japanese friends won't need to continue their current hacks).

basically the JIT we have operates on loops, and the code uses a RegExp engine instead of a classical loop. We could ask the JIT people about an ETA for supporting this, or I could swap to a for loop. The for loop is easy enough.

Anyway, here's a basic impl and a test driver in the commit message.

For that test drive, I get this:
recorder: started(57), aborted(29), completed(58), different header(1), trees trashed(0), slot promoted(0), unstable loop variable(8), breaks(25), returns(0), unstableInnerCalls(4)
monitor: triggered(143), exits(137), type mismatch(0), global mismatch(2)

Which i'd hope is better than nothing.

I'm hoping jorendorff or someone can poke the JIT for me while I spend my weekend.

Comment 32

[Brendan Eich [:brendan]]

timeless: we have a regexp JIT too. But it handles only a subset of the regexp language -- what are you using?

/be

Comment 33

[timeless]

brendan: do you have a spec?

it's in the attachments: /%(?:(\d+)\$|)(?:(\d*)\.(\d*)|)([%SscdxX])/g

and please note that i don't really have the energy to tune this. i want it fixed and you want us to reduce our c++ usage, so i'd hope you could help.

adding:
StringBundle.prototype.formatString = StringBundle.prototype.formatStringRegExp

before calling the testService line and proceeding w/ the same testing as w/ the patch comment yields:

recorder: started(10), aborted(9), completed(1), different header(0), trees trashed(0), slot promoted(0), unstable loop variable(0), breaks(0), returns(0), unstableInnerCalls(0)
monitor: triggered(3), exits(3), type mismatch(0), global mismatch(1)

so, my guess is that the only thing it compiled was the for loop for the testing. Also, please note that this bug is dedicated toward actually fixing the stringbundle code. I don't want it to get sidetracked by adventures in JIT support land. If you want a bug to investigate, please feel free to file it.

Note: I haven't actually done performance testing w/ this variant, I'm posting it so that someone else can compare and try to help get something into the tree.

pike: we're not talking about a side by side impl, we're talking about deleting the c++ impl and replacing it with the js impl. C++ callers wouldn't be able to crash.

Comment 34

[Brendan Eich [:brendan]]

We don't trace capturing parens. The spec is the code, or maybe a bug has a separate spec -- dmandelin can say.

I'm not able to help right now, sorry. Maybe after Firefox 3.5. You can probably get help from others too, in that timeframe. In the mean time let's focus on what we can do with the JIT as it will be in 3.5.

/be

Comment 35

[timeless]

Attachment: updated

this includes all the build integration bits, some minor changes which should hopefully help spidermonkey make things faster, and an edge case bug fix.

I've also wrapped the Properties constructor in a Proxy because the Properties object isn't threadsafe. However, it doesn't work correctly because the xpcom proxy impl we have is broken (it doesn't tag itself as threadsafe), that's an old bug I keep hitting, I'm going to look at it now....

Comment 36

[Axel Hecht [:Pike]]

Comment on attachment 375213 [details] [diff] [review]
updated

Can we get talos try results for this with and without chrome jit on? Thanks.

Comment 37

[Axel Hecht [:Pike]]

Comment on attachment 375213 [details] [diff] [review]
updated

Cancelling review request until we have performance data.

Comment 38

[timeless]

you'll have to find someone else to do that. sorry.

Comment 39

[Axel Hecht [:Pike]]

I'm pretty sure that we fixed all of these in bug 1388789.