Last Comment Bug 743056 - nsPrintfCString should always require a buffer size
: nsPrintfCString should always require a buffer size
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: String (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
:
Mentors:
: 261145 (view as bug list)
Depends on:
Blocks: 745659
  Show dependency treegraph
 
Reported: 2012-04-05 16:20 PDT by Vladan Djeric (:vladan)
Modified: 2016-05-09 19:21 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch to remove the "default" constructor (3.33 KB, patch)
2012-04-06 10:14 PDT, Nathan Froyd [:froydnj]
no flags Details | Diff | Splinter Review
content/ changes (2.04 KB, patch)
2012-04-06 10:26 PDT, Nathan Froyd [:froydnj]
mounir: review+
Details | Diff | Splinter Review
dom/ changes (1.88 KB, patch)
2012-04-06 10:27 PDT, Nathan Froyd [:froydnj]
bzbarsky: review+
Details | Diff | Splinter Review
toolkit/ changes (20.03 KB, patch)
2012-04-06 10:28 PDT, Nathan Froyd [:froydnj]
dtownsend: review+
Details | Diff | Splinter Review
layout/ changes (4.42 KB, patch)
2012-04-06 10:29 PDT, Nathan Froyd [:froydnj]
no flags Details | Diff | Splinter Review
widget/ changes (6.46 KB, patch)
2012-04-06 10:30 PDT, Nathan Froyd [:froydnj]
roc: review+
Details | Diff | Splinter Review
netwerk/ changes (5.94 KB, patch)
2012-04-06 10:31 PDT, Nathan Froyd [:froydnj]
no flags Details | Diff | Splinter Review
security/manager/ changes (2.20 KB, patch)
2012-04-06 10:31 PDT, Nathan Froyd [:froydnj]
honzab.moz: review+
Details | Diff | Splinter Review
xpcom/ changes (1.79 KB, patch)
2012-04-06 10:32 PDT, Nathan Froyd [:froydnj]
no flags Details | Diff | Splinter Review
gfx/ changes (1.17 KB, patch)
2012-04-06 10:32 PDT, Nathan Froyd [:froydnj]
jmuizelaar: review-
Details | Diff | Splinter Review
js/xpconnect/ changes (1.36 KB, patch)
2012-04-06 10:33 PDT, Nathan Froyd [:froydnj]
bobbyholley: review+
Details | Diff | Splinter Review
storage/ changes (3.24 KB, patch)
2012-04-06 10:34 PDT, Nathan Froyd [:froydnj]
mak77: review+
Details | Diff | Splinter Review
toolkit/ changes (20.02 KB, patch)
2012-04-06 12:30 PDT, Nathan Froyd [:froydnj]
mak77: review+
Details | Diff | Splinter Review
Do the obvious thing, allocate, rev. 1 (4.98 KB, patch)
2012-04-09 09:41 PDT, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
justin.lebar+bug: review-
Details | Diff | Splinter Review
Do the obvious thing, allocate, rev. 2 (14.14 KB, patch)
2012-04-09 11:20 PDT, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
justin.lebar+bug: review+
Details | Diff | Splinter Review
Do the obvious thing, allocate, rev. 2.1 (14.27 KB, patch)
2012-04-12 10:29 PDT, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
no flags Details | Diff | Splinter Review

Description Vladan Djeric (:vladan) 2012-04-05 16:20:23 PDT
Currently, nsPrintfCString supplies two constructors: one takes a buffer size argument specifying the maximum output string length and one which (silently) assumes a maximum output length of 15 bytes.

The second version of the constructor is problematic because the 15 byte max behaviour is likely not what the nsPrintfCString users are expecting. For example, in bug 741821, :espindola found that SQL strings prefixed with comments were getting truncated by nsPrintfCString, resulting in no-op SQL statements and NULL pointer fun.

We should remove the problematic constructor and rewrite all calls to it to use the other c'tor and explicitly specify an output buffer length.

Assigning to froydnj at Taras's suggestion.
Comment 1 Nathan Froyd [:froydnj] 2012-04-06 10:14:09 PDT
Created attachment 612921 [details] [diff] [review]
patch to remove the "default" constructor

The trivial patch to remove the default size 15 constructor.

This and all subsequent patches will of course be rolled up for commit; the separation is just for ease of review.
Comment 2 Nathan Froyd [:froydnj] 2012-04-06 10:19:59 PDT
I'll describe my methodology for the subsequent patches once, here, so I can refer to a single location and avoiding spamming the bug.

By and large, I simply changed:

nsPrintfCString foo(fmt, ...);

to

nsPrintfCString foo(15, fmt, ...);

as that gives the behavior things had before.  Exceptions:

- Where 15 looked "obviously" bad, I bumped it up.  Generally anything with two or more %letter sequences got a bump.  If the string was already pushing 15 characters, I bumped it up too.
- I used a simple AppendInt where appropriate, which may result in slightly longer code.
- Likewise for AppendPrintf, but this is rare.

I took the liberty of deleting dead nsPrintfCString.h includes where appropriate.

Patch spam incoming.
Comment 3 Nathan Froyd [:froydnj] 2012-04-06 10:26:38 PDT
Created attachment 612927 [details] [diff] [review]
content/ changes
Comment 4 Nathan Froyd [:froydnj] 2012-04-06 10:27:54 PDT
Created attachment 612928 [details] [diff] [review]
dom/ changes
Comment 5 Nathan Froyd [:froydnj] 2012-04-06 10:28:45 PDT
Created attachment 612929 [details] [diff] [review]
toolkit/ changes
Comment 6 Nathan Froyd [:froydnj] 2012-04-06 10:29:52 PDT
Created attachment 612931 [details] [diff] [review]
layout/ changes
Comment 7 Nathan Froyd [:froydnj] 2012-04-06 10:30:38 PDT
Created attachment 612932 [details] [diff] [review]
widget/ changes
Comment 8 Nathan Froyd [:froydnj] 2012-04-06 10:31:13 PDT
Created attachment 612933 [details] [diff] [review]
netwerk/ changes
Comment 9 Nathan Froyd [:froydnj] 2012-04-06 10:31:48 PDT
Created attachment 612934 [details] [diff] [review]
security/manager/ changes
Comment 10 Nathan Froyd [:froydnj] 2012-04-06 10:32:20 PDT
Created attachment 612935 [details] [diff] [review]
xpcom/ changes
Comment 11 Nathan Froyd [:froydnj] 2012-04-06 10:32:54 PDT
Created attachment 612936 [details] [diff] [review]
gfx/ changes
Comment 12 Nathan Froyd [:froydnj] 2012-04-06 10:33:46 PDT
Created attachment 612937 [details] [diff] [review]
js/xpconnect/ changes
Comment 13 Boris Zbarsky [:bz] 2012-04-06 10:33:58 PDT
Comment on attachment 612928 [details] [diff] [review]
dom/ changes

r=me
Comment 14 Nathan Froyd [:froydnj] 2012-04-06 10:34:17 PDT
Created attachment 612938 [details] [diff] [review]
storage/ changes
Comment 15 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-06 11:23:46 PDT
Comment on attachment 612938 [details] [diff] [review]
storage/ changes

Review of attachment 612938 [details] [diff] [review]:
-----------------------------------------------------------------

::: storage/src/VacuumManager.cpp
@@ +230,5 @@
>    // vacuum, if they are executed in the same transaction.
>    nsCOMPtr<mozIStorageAsyncStatement> pageSizeStmt;
> +
> +  nsCAutoString pragma_str;
> +  pragma_str.Append(MOZ_STORAGE_UNIQUIFY_QUERY_STR "PRAGMA page_size = ");

You may probably initialize the autostring to a value, instead of creating it empty and then appending immediately
Comment 16 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-06 11:45:35 PDT
Comment on attachment 612929 [details] [diff] [review]
toolkit/ changes

Review of attachment 612929 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsNavHistory.cpp
@@ +3969,5 @@
>      mClause.Append(' ');
>      if (!mQueryIndex)
>        mClause.Append(aParam);
>      else
> +      mClause += nsPrintfCString(15, "%s%d", aParam, mQueryIndex);

we are quite near the limit here (I've found a couple 14 sized strings, so it's working by luck), I'd prefer a bit larger buffer, or we should rather append.
Comment 17 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-06 11:46:23 PDT
May we add a debug-only fatal assert on the string length vs the buffer size? Even if that may we a bit more expensive, would be debug-only and at least some sort of protection.
Comment 18 Nathan Froyd [:froydnj] 2012-04-06 11:49:23 PDT
(In reply to Marco Bonardo [:mak] from comment #16)
> ::: toolkit/components/places/nsNavHistory.cpp
> @@ +3969,5 @@
> >      mClause.Append(' ');
> >      if (!mQueryIndex)
> >        mClause.Append(aParam);
> >      else
> > +      mClause += nsPrintfCString(15, "%s%d", aParam, mQueryIndex);
> 
> we are quite near the limit here (I've found a couple 14 sized strings, so
> it's working by luck), I'd prefer a bit larger buffer, or we should rather
> append.

This would be a good place to AppendPrintf; I should have done that in the first place.  I'll update the patch.
Comment 19 Boris Zbarsky [:bz] 2012-04-06 11:54:16 PDT
Comment on attachment 612928 [details] [diff] [review]
dom/ changes

Actually, for that first one, using 23 might be better, since a PRUint32 _could_ end up 10 digits long....
Comment 20 Dave Townsend [:mossop] 2012-04-06 12:08:48 PDT
Comment on attachment 612929 [details] [diff] [review]
toolkit/ changes

Marco, can you look over the places pieces of this too please, the rest looks fine
Comment 21 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-06 12:10:14 PDT
sure, the next version that will implement comment 18 :)
Comment 22 Nathan Froyd [:froydnj] 2012-04-06 12:28:16 PDT
(In reply to Boris Zbarsky (:bz) from comment #19)
> Actually, for that first one, using 23 might be better, since a PRUint32
> _could_ end up 10 digits long....

Would you object to just changing that to:

  ver.Append('/')
  ver.AppendInt(major)
  ver.Append('.')
  ver.AppendInt(minor)

?
Comment 23 Boris Zbarsky [:bz] 2012-04-06 12:28:46 PDT
Not at all.  Looks good to me.
Comment 24 Nathan Froyd [:froydnj] 2012-04-06 12:30:21 PDT
Created attachment 612976 [details] [diff] [review]
toolkit/ changes

Implementing comment 18 and handing off to mak.
Comment 25 Nathan Froyd [:froydnj] 2012-04-06 12:40:02 PDT
(In reply to Marco Bonardo [:mak] from comment #17)
> May we add a debug-only fatal assert on the string length vs the buffer
> size? Even if that may we a bit more expensive, would be debug-only and at
> least some sort of protection.

As in, "assert if the producing string from printf is longer than the provided buffer"?  There doesn't seem to be a good way to do that; PR_{v,}snprintf don't tell you if they overflowed the buffer.  {v,}snprintf *do* signal that, however.  I'd like to ditch NSPR bits, but whether {v,}snprintf are portable and don't cause weird behavior changes...
Comment 26 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-06 12:57:07 PDT
(In reply to Nathan Froyd (:froydnj) from comment #25)
> As in, "assert if the producing string from printf is longer than the
> provided buffer"?  There doesn't seem to be a good way to do that;
> PR_{v,}snprintf don't tell you if they overflowed the buffer.  {v,}snprintf
> *do* signal that, however.  I'd like to ditch NSPR bits, but whether
> {v,}snprintf are portable and don't cause weird behavior changes...

I see, maybe worth a follow-up in nspr to investigate adding overflow detection?
Otherwise, we may "guess" from the initial string and the kind of bound data if there's opportunity to go over the size, would be quite far from perfect, but it's scary that we don't have any sort of check here :(
Comment 27 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-06 13:36:30 PDT
Comment on attachment 612976 [details] [diff] [review]
toolkit/ changes

Review of attachment 612976 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsNavHistory.cpp
@@ +249,5 @@
>             "FROM moz_bookmarks b_t "
>             "JOIN moz_bookmarks t_t ON t_t.id = +b_t.parent  "
>             "WHERE b_t.fk = ") + aRelation + NS_LITERAL_CSTRING(" "
>             "AND t_t.parent = ") +
> +           nsPrintfCString(15, "%lld", aTagsFolder) + NS_LITERAL_CSTRING(" "

nit: int64 may go up to 20 chars, though it's really unlikely we may ever hit that, also cause it would cause much bigger pain in js with the 53 bits limit. This may also be valid below for %d, where you usually accounted 10 chars, but not everywhere. Again, it's mostly cases where we are unlikely to go over 10, so I don't care that much.

::: toolkit/xre/nsAppRunner.cpp
@@ +1928,5 @@
>  
>    nsCOMPtr<nsIToolkitProfile> newProfile;
>    // Make the new profile "default-" + the time in seconds since epoch for uniqueness.
>    nsCAutoString newProfileName("default-");
> +  newProfileName.AppendInt(PR_Now() / 1000);

may this implicit cast warn?
Comment 28 Justin Lebar (not reading bugmail) 2012-04-06 14:29:57 PDT
Why can't we just make this do the right thing and always allocate enough space?  The comment in the file says it's not intended for long strings, but it's clearly being used that way.

We should just write an equivalent of vasprintf and be done with it.  In the meantime, perhaps we should make nsPrintfCString debug-assert when you exceed its bounds?
Comment 29 Justin Lebar (not reading bugmail) 2012-04-06 14:34:21 PDT
> In the meantime, perhaps we should make nsPrintfCString debug-assert when you exceed its bounds?

Sorry, I should have read the bug first.  :)  It's a stupid bug in NSPR that their snprintf doesn't return the size!  But let's please not stop at this bug in fixing this.
Comment 30 :Ms2ger 2012-04-07 04:28:57 PDT
Comment on attachment 612927 [details] [diff] [review]
content/ changes

Review of attachment 612927 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/document/src/nsHTMLDocument.cpp
@@ +2151,1 @@
>        + NS_LITERAL_CSTRING("/")

I think this should just use Append...
Comment 31 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-04-09 09:41:24 PDT
Created attachment 613321 [details] [diff] [review]
Do the obvious thing, allocate, rev. 1

I'm pretty sure that we don't need any of the other changes on this bug if we just fix nsPrintfCstring to allocate, which I've done using the PR_vsxprintf function. I also converted nsPrintfCString to be a true fixed-string class, now that we can do that basically for free.
Comment 32 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-04-09 10:26:32 PDT
https://tbpl.mozilla.org/?tree=Try&rev=c50ce57fe330 submitted to try.
Comment 33 Justin Lebar (not reading bugmail) 2012-04-09 10:28:51 PDT
Comment on attachment 613321 [details] [diff] [review]
Do the obvious thing, allocate, rev. 1

>+++ b/xpcom/string/public/nsPrintfCString.h
>@@ -48,40 +48,42 @@
>    *
>    *   myCStr += nsPrintfCString("%f", 13.917);
>    *     // ...a general purpose substitute for |AppendFloat|
>    *
>    * For longer patterns, you'll want to use the constructor that takes a length
>    *
>    *   nsPrintfCString(128, "%f, %f, %f, %f, %f, %f, %f, %i, %f", x, y, z, 3.2, j, k, l, 3, 3.1);
>    *
>-   * Exceding the default size (which you must specify in the constructor, it is not determined)
>-   * causes an allocation, so avoid that.  If your formatted string exceeds the allocated space, it is
>-   * cut off at the size of the buffer, no error is reported (and no out-of-bounds writing occurs).
>-   * This class is intended to be useful for numbers and short
>-   * strings, not arbitrary formatting of other strings (e.g., with %s).  There is currently no
>-   * wide version of this class, since wide |printf| is not generally available.  That means
>-   * to get a wide version of your formatted data, you must, e.g.,
>+   * There is currently no wide version of this class, since wide |printf| is
>+   * not generally available.  That means to get a wide version of your
>+   * formatted data, you must, e.g.,
>    *
>    *   CopyASCIItoUTF16(nsPrintfCString("%f", 13.917"), myStr);
>    *
>    * That's another good reason to avoid this class for anything but numbers ... as strings can be
>    * much more efficiently handled with |NS_LITERAL_[C]STRING| and |nsLiteral[C]String|.
>    */

I'd like us to clean up the comment about how we shouldn't use this class for anything but numbers, since that is consistent with neither its new design nor how it's used.  :)

>-// though these classes define a fixed buffer, they do not set the F_FIXED
>-// flag.  this is because they are not intended to be modified after they have
>-// been constructed.  we could change this but it would require adding a new
>-// class to the hierarchy, one that both this class and nsCAutoString would
>-// inherit from.  for now, we populate the fixed buffer, and then let the
>-// nsCString code treat the buffer as if it were a dependent buffer.
>-
> nsPrintfCString::nsPrintfCString( const char_type* format, ... )
>-  : string_type(mLocalBuffer, 0, F_TERMINATED)
>+  : nsFixedCString(mLocalBuffer, kLocalBufferSize)
>   {

Looks like this will cause nsTFixedString to do a strlen on mLocalBuffer.

|nsFixedCString(mLocalBuffer, kLocalBufferSize, 0)| should be safe, but perhaps we should just inherit from nsCAutoString?
Comment 34 Nathan Froyd [:froydnj] 2012-04-09 10:36:01 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #31)
> I'm pretty sure that we don't need any of the other changes on this bug if
> we just fix nsPrintfCstring to allocate, which I've done using the
> PR_vsxprintf function. I also converted nsPrintfCString to be a true
> fixed-string class, now that we can do that basically for free.

If we're going to do that, why not just nuke nsPrintfCString and adjust places to use AppendPrintf instead?
Comment 35 Justin Lebar (not reading bugmail) 2012-04-09 10:42:31 PDT
> If we're going to do that, why not just nuke nsPrintfCString and adjust places to use AppendPrintf 
> instead?

AppendPrintf currently allocates, but we could definitely change that.  (Also, a short-lived allocation like this is not necessarily the end of the world.)
Comment 36 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-04-09 11:20:15 PDT
Created attachment 613346 [details] [diff] [review]
Do the obvious thing, allocate, rev. 2

This moves all of the interesting logic into the nsTSubstring::AppendPrintf methods and just makes nsPrintfCString a sugaring syntax.
Comment 37 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-04-09 11:24:23 PDT
Also, I didn't just inherit from nsCAutoString because the builtin buffer sizes were different. I don't think it matters much, though...
Comment 38 Jeff Muizelaar [:jrmuizel] 2012-04-09 11:33:52 PDT
Comment on attachment 612936 [details] [diff] [review]
gfx/ changes

I too would prefer AppendPrintf here.
Comment 39 Justin Lebar (not reading bugmail) 2012-04-09 11:42:06 PDT
Comment on attachment 613346 [details] [diff] [review]
Do the obvious thing, allocate, rev. 2

>-    private:
>-      char_type  mLocalBuffer[ kLocalBufferSize + 1 ];
>+      char_type  mLocalBuffer[ kLocalBufferSize ];
>   };
> 
>+
>+

Nit: extra whitespace
Comment 40 Mounir Lamouri (:mounir) 2012-04-10 06:55:05 PDT
Comment on attachment 612927 [details] [diff] [review]
content/ changes

Review of attachment 612927 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/document/src/nsHTMLDocument.cpp
@@ +2146,5 @@
>    mDocumentURI->GetSpec(originalSpec);
>  
>    // Generate the wyciwyg url
>    url = NS_LITERAL_CSTRING("wyciwyg://")
> +      + nsPrintfCString(15, "%d", gWyciwygSessionCnt++)

gWyciwygSessionCnt would be 10 digits long max so I guess you could use 10 instead of 15 here. 
In addition, you could use AppendInt instead of nsPrintfCString as Ms2ger pointed out. But I'm fine either way.
Comment 41 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-04-12 10:29:44 PDT
Created attachment 614436 [details] [diff] [review]
Do the obvious thing, allocate, rev. 2.1

got r=jlebar over IRC for this slight modification: NSPR hands stufffunc the terminating null,  and our strings don't want that
Comment 42 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-04-13 10:38:08 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3f9ee8bb745

I don't think we need to bother with most of the rest of these patches: were there any correctness changes included instead of just changing the calling patterns?
Comment 43 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-14 06:43:58 PDT
https://hg.mozilla.org/mozilla-central/rev/a3f9ee8bb745
Comment 44 Nathan Froyd [:froydnj] 2012-04-16 10:58:07 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #42)
> I don't think we need to bother with most of the rest of these patches: were
> there any correctness changes included instead of just changing the calling
> patterns?

There were, but only because of too-short specified lengths.  If we don't use the length now, then all reason for any of the patches is gone.
Comment 45 :Cykesiopka 2016-05-09 19:21:53 PDT
*** Bug 261145 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.