743056 - nsPrintfCString should always require a buffer size

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Vladan Djeric (:vladan)]

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

Attachment: 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]]

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

Attachment: content/ changes

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: dom/ changes

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: toolkit/ changes

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: layout/ changes

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: widget/ changes

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: netwerk/ changes

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: security/manager/ changes

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: xpcom/ changes

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: gfx/ changes

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: js/xpconnect/ changes

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 612928 [details] [diff] [review]
dom/ changes

r=me

Comment 14

[Nathan Froyd [:froydnj]]

Attachment: storage/ changes

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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 Apr 25 - May 5)]

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 Apr 25 - May 5)]

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

(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 [:bzbarsky]]

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

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 Apr 25 - May 5)]

sure, the next version that will implement comment 18 :)

Comment 22

[Nathan Froyd [:froydnj]]

(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 [:bzbarsky]]

Not at all.  Looks good to me.

Comment 24

[Nathan Froyd [:froydnj]]

Attachment: toolkit/ changes

Implementing comment 18 and handing off to mak.

Comment 25

[Nathan Froyd [:froydnj]]

(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 Apr 25 - May 5)]

(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 Apr 25 - May 5)]

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)]

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)]

> 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 (he/him; ⌚ UTC+1/+2)]

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]

Attachment: 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]

https://tbpl.mozilla.org/?tree=Try&rev=c50ce57fe330 submitted to try.

Comment 33

[Justin Lebar (not reading bugmail)]

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

(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)]

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

Attachment: 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]

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

Comment on attachment 612936 [details] [diff] [review]
gfx/ changes

I too would prefer AppendPrintf here.

Comment 39

[Justin Lebar (not reading bugmail)]

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)]

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]

Attachment: 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]

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 Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/a3f9ee8bb745

Comment 44

[Nathan Froyd [:froydnj]]

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