438688 - String formatter fails to format the same argument twice

Status: RESOLVED FIXED

Description

[Mike Hommey [:glandium]]

With strings such as "The certificate is only valid for <a id="cert_domain_link" title="%1$S">%1$S</a>", where the first argument is supposed to be formatted twice, the second occurrence is replaced by "(null)", at least on amd64 under linux. This is due to the way va_lists are being used in dosprintf in nsTextFormatter.cpp.
It makes the neterror page for bad certificate read "The certificate is only valid for (null)", which is pretty useless to the user (though hovering displays the correct url thanks to the <a title> around "(null)")
While the first time it is called, u.S = va_arg(ap, const PRUnichar*); gives the proper pointer, the second time, it returns NULL. I guess this does the same on any arch using registers for arguments in function calls.
The attached patch alleviates the issue, but i suspect it might leak some memory (depending on how va_copy is implemented).
Note that there is already a call to va_copy in BuildArgArray, without a corresponding va_end (which is supposed to be mandatory), so it may leak there as well. Note that I don't understand why NumArgState keeps va_lists, while keeping a list of arguments (which you almost build in BuildArgArray) would work, too, provided a union is used in NumArgState. That would avoid va_copys and the problem reported here.

Comment 1

[Mike Hommey [:glandium]]

Created attachment 324679 [details] [diff] [review]
patch

Comment 2

[Alexander Sack]

what about other implementations/copies of this dosprintf code?

I see: mozilla/js/src/jsprf.c, mozilla/nsprpub/pr/src/io/prprf.c and  mozilla/xpcom/glue/nsTextFormatter.cpp.

Comment 3

[Mike Hommey [:glandium]]

Do they need to handle cases where the same argument is formatted several times by the format string ?
Also, if the code is the same (I haven't looked), why not have it in nspr only ? code duplication is bad.

Comment 4

[Benjamin Smedberg]

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

I'd like a unit test, please.

Comment 5

[Mike Hommey [:glandium]]

Created attachment 325603 [details] [diff] [review]
Same patch, with testcase

The modified test "properly" fails with older nsTextFormatter.cpp and succeeds with the patched one.

Comment 6

[Dão Gottwald [::dao]]

http://hg.mozilla.org/index.cgi/mozilla-central/rev/c333478161f6