stringbundle can crash if given bad input

Assigned to



14 years ago
6 years ago


(Reporter: timeless, Assigned: smontagu)


(Depends on: 1 bug, Blocks: 1 bug, {crash, qawanted})

Windows XP
crash, qawanted
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [needs performance data])


(1 attachment, 8 obsolete attachments)



14 years ago
%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

14 years ago
Created attachment 179146 [details] [diff] [review]

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


Is that as expected?
Attachment #179146 - Flags: superreview?(
Attachment #179146 - Flags: review?(timeless)

Comment 2

14 years ago
Comment on attachment 179146 [details] [diff] [review]

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

Comment 3

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

Comment 4

10 years ago
Created attachment 370154 [details] [diff] [review]

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
Attachment #370154 - Flags: review?(smontagu)

Comment 5

10 years ago
...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

10 years ago
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

10 years ago
Created attachment 370377 [details] [diff] [review]
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]
Attachment #370154 - Attachment is obsolete: true
Attachment #370377 - Flags: review?(smontagu)
Attachment #370154 - Flags: review?(smontagu)


10 years ago
Attachment #370377 - Attachment is patch: true
Attachment #370377 - Attachment mime type: application/x-javascript → text/plain


10 years ago
Attachment #370154 - Attachment is patch: true


10 years ago
Depends on: 486311

Comment 8

10 years ago
Created attachment 370411 [details] [diff] [review]

ok, this component seems to work, note that you must apply the patch from bug 486311 because of a bug in caps...
Attachment #370377 - Attachment is obsolete: true
Attachment #370411 - Flags: review?(smontagu)
Attachment #370377 - Flags: review?(smontagu)

Comment 9

10 years ago
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

10 years ago
> bdl=Components.classes[";1"].getService(Components.interfaces.nsIStringBundleService).createBundle("data:text/plain,a=%2$S").formatStringFromName("a",["hello","world"],2) 

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[";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

10 years ago
Created attachment 370451 [details]
support override service

ok, with this, all that's left is deleting the c++ directory and the references in build
Assignee: smontagu → timeless
Attachment #370411 - Attachment is obsolete: true
Attachment #370451 - Flags: review?(smontagu)
Attachment #370411 - Flags: review?(smontagu)
QA Contact: amyy → i18n
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.
... which doesn't actually matter since it rolls its own formatting.

Comment 14

10 years ago
Created attachment 370826 [details] [diff] [review]
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.

    * 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. 
    * line 109 -- 4009=Received %ld of %ld messages
    * line 363 -- percent=%S%%
    * 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?*[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,

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.
Attachment #370451 - Attachment is obsolete: true
Attachment #370826 - Flags: superreview?(neil)
Attachment #370826 - Flags: review?(smontagu)
Attachment #370451 - Flags: review?(smontagu)

Comment 15

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

Comment 16

10 years ago
what does a zero width string mean?

Comment 17

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

Comment 18

10 years ago
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

10 years ago
(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?


> 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.
(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

10 years ago has the only ones I know about.

I don't see any obvious candidates for the stringbundle xul element.
(In reply to comment #21)
> 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.,544413&sel=1238562078,1238767486

here are the two mac 10.4 try builders with Ts graphed out:,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.[{%22test%22:%2212%22,%22branch%22:%226%22,%22machine%22:%22130%22}][{%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.
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

10 years ago
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?)
Attachment #370826 - Flags: review?(smontagu) → review?(l10n)

Comment 26

10 years ago
Created attachment 371125 [details] [diff] [review]
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[";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, */
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.
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).
Attachment #370826 - Attachment is obsolete: true
Attachment #371125 - Flags: superreview?(neil)
Attachment #371125 - Flags: review?
Attachment #370826 - Flags: superreview?(neil)
Attachment #370826 - Flags: review?(l10n)


10 years ago
Attachment #371125 - Flags: review? → review?(l10n)
this morning's try build (with patch from c#26), passes unittests, but does exhibit some crashing on exit. not sure if that's not shutting down properly or what.

still waiting on talos runs. Note that these are using default prefs, i.e., no chrome jit.[{%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.[{%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.[{%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:[{%22test%22:%222%22,%22branch%22:%226%22,%22machine%22:%22130%22}]&sel=1238612835,1239018960

Comment 30

10 years ago
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

10 years ago
Created attachment 374470 [details] [diff] [review]
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.
Attachment #374470 - Flags: review?(jorendorff)
timeless: we have a regexp JIT too. But it handles only a subset of the regexp language -- what are you using?


Comment 33

10 years ago
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.

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.
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.



10 years ago
Attachment #374470 - Flags: review?(rcampbell)

Comment 35

10 years ago
Created attachment 375213 [details] [diff] [review]

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....
Attachment #179146 - Attachment is obsolete: true
Attachment #371125 - Attachment is obsolete: true
Attachment #374470 - Attachment is obsolete: true
Attachment #375213 - Flags: superreview?(neil)
Attachment #375213 - Flags: review?(l10n)
Attachment #374470 - Flags: review?(rcampbell)
Attachment #374470 - Flags: review?(jorendorff)
Attachment #371125 - Flags: superreview?(neil)
Attachment #371125 - Flags: review?(l10n)
Attachment #179146 - Flags: superreview?(neil)
Attachment #179146 - Flags: review?(timeless)

Comment 36

10 years ago
Comment on attachment 375213 [details] [diff] [review]

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

Comment 37

10 years ago
Comment on attachment 375213 [details] [diff] [review]

Cancelling review request until we have performance data.
Attachment #375213 - Flags: review?(l10n)


8 years ago
Blocks: 658766

Comment 38

8 years ago
you'll have to find someone else to do that. sorry.
Assignee: timeless → smontagu


6 years ago
Keywords: qawanted
Whiteboard: [needs performance data]
You need to log in before you can comment on or make changes to this bug.