229662 - PR_snprintf does not return a useful error value

Status: NEW

(NSPR :: NSPR, defect)

Description

[tenthumbs]

prprf.h claims that PR_snprintf returns (PRUint32)(-1) on an error. The 
most common (and maybe the only) error is a possible buffer overrun. 
PR_snprintf does not treat this situation as an error. It appears to 
always return the number of bytes actually written so there is no way at 
all to detect a potential overrun.

The documented behavior is appropriate for an older snprintf 
implementation. ISO C99 requires snprintf to return the number of bytes 
that would have been written if the buffer was large enough. PR_snprintf 
should do one of these two things.

A later note. One would think that PR_snprintf would return buffer_len -1. 
It actually writes that many bytes but returns buffer_len -2.

Comment 1

[timeless]

see also bug 162786

Comment 2

[Malcolm Rowe]

Attachment: Update PR_snprintf to ISO C99

This patch updates PR_snprintf to follow ISO C99 - return -1 for errors (like
invalid format specifiers), and the number of characters generated otherwise
(excluding the NUL-terminator). Also following ISO C99, it allows 'out' to be a
NULL pointer, if 'outlen' is zero.

It also changes prftest.c to include more tests for the various conditions.

As the reporter states, the return value of PR_snprintf is currently useless,
so this should not affect any current callers (as any callers that currently
trust the return value must be broken anyway).

Comment 3

[Malcolm Rowe]

Comment on attachment 146718 [details] [diff] [review]
Update PR_snprintf to ISO C99

Requesting r= for this patch. Does NSPR need sr=?

Note: there's some user-visible documentation available at
http://www.mozilla.org/projects/nspr/reference/html/prprf.html#23391 that
should also be updated. I don't know whether this is generated from somewhere,
and if so, from where.

Comment 4

[tenthumbs]

There's no way to distinguish an error condition from a large total size. In 
ISO 
C99 you do 

    count = snprint (buffer, buffer_len, format, ...);
    if (count >= buffer_len)
        resize buffer and try again or bail out.

If you want PR_snprint to behave similary then returning -1 on error is just a
huge unsigned number. How would you know the difference. I think you just have 
to handle any errors silently.

BTW, this bug came out of bug 186547 where the caller really did want to 
handle overruns.

Comment 5

[Malcolm Rowe]

ISO C99 snprintf returns an int, which can be negative in the case of 'an
encoding error' (typically an invalid format specifier, or an attempt to use an
invalid multibyte sequence, for example 0x80 0x80 in a UTF-8 locale).

From C99: "The snprintf function returns the number of characters that would
have been written had n been sufficiently large, not counting the terminating
null character, or a negative value if an encoding error occurred. Thus, the
null-terminated output has been completely written if and only if the returned
value is nonnegative and less than n."

For NSPR, the return value of PR_snprintf is a PRUint32. Changing that to a
PRInt32 would mean changing the interface, which might affect current callers,
so in NSPR, the error return is only ever (PRUint32)-1.

So, while in C99 you do:
  int ret = snprintf ...;
  if (ret >= 0 && ret < n) { ok }
In NSPR, the equivalent is:
  PRUint32 ret = PR_snprintf ...;
  if (ret != (PRUint32)-1 && ret < n) { ok }

[Does NSPR require a stable binary ABI? I thought it did. If not, changing to a
PRInt32 per ISO C99 would be a really good idea.]

So, no, in NSPR you won't be able to distinguish a (2^32-1) write from an error
condition. However, in C99. any writes over 2^31 are 'undefined' anyway (since
'n' in size_t, but the return is int).

Bear in mind that PR_snprintf has always had this documented behaviour - it just
didn't follow it before.

Or were you saying that you now need to explicitly test for an error to
distinguish it from a buffer overflow? If so, I agree that's a pain.

wtc: can we change the return type to PRInt32 to follow ISO C99?

Comment 6

[Malcolm Rowe]

A quick survey of in-tree callers reveals:

* No callers of PR_vsnprintf or PR_snprintf check for an overrun indicated by 
ret >= bufsize (unsurprisingly, since they can't currently return a value 
indicating truncation).

* Some callers already check for a -1 return to indicate an error; see for 
example
http://lxr.mozilla.org/mozilla/source/gfx/src/mac/nsUnicodeRenderingToolkit.cpp#
927

* Some callers don't check for any error, and just pass the return value 
directly to a Write() call (typically); for example, this snippet 
from /extensions/cookie/nsPermissionManager.cpp:

923         PRUint32 len = PR_snprintf(permissionString, sizeof
(permissionString) - 1, "%u", permission);
924         bufferedOutputStream->Write(permissionString, len, &rv);

With the change in the attached patch, these callers might attempt to write 
more data than was actually stored in the buffer, or an extremely large value 
in case of an error. I guess that we should probably update the callers to 1) 
check for an error, and 2) check for an overrun.

[Also note the use of sizeof(buf)-1 rather than sizeof(buf) - that's fairly 
common too, but not harmful.]

* Some callers (incorrectly) treat the return value of PR_snprintf as a signed 
value. For example,

/intl/unicharutil/src/nsSaveAsCharset.cpp
344       rv = (PR_snprintf(outString, bufferLength, "\\u%.6x", inUCS4) > 0) ? 
NS_OK : NS_ERROR_FAILURE;

/mailnews/mime/src/comi18n.cpp
231     if (PR_snprintf(encodedword_head, sizeof(encodedword_head)-1, "=?%s?%
c?", charset, method) < 0) {

* /js/src/jsprf.c appears to be an auto-generated duplicate 
of /nsprpub/pr/io/prprf.c. cc'ing brendan to find out if we would need to 
regenerate the former from the latter.

Comment 7

[tenthumbs]

Testing for -1 is the currently documented behavior but the C99 behavior is
what programmers are (or will be) accustomed to. I think you want to copy the
C99 behavior or you risk incorrect code because a programmer had an off day or
didn't bother reading the docs.  Just look at the various ways people use the
function.

Since any useful change redefines the interface somewhat I would go with 
returning and int and _documenting_ it well.

Comment 8

[Malcolm Rowe]

tenthumbs: I agree completely, but I want feedback from NSPR peers (presumably 
wtc) on whether we can safely change the interface of the function to return 
PRInt32.

I'm happy to review the in-tree callers of PR_snprintf and PR_vsnprintf and fix 
as appropriate, but I don't know about possible external users of NSPR, and 
whether changing the return value from PRUint32 to PRInt32 would break some 
kind of binary compatibility that we might care about (although I don't think 
that it should really affect an ABI, thinking about it).

If I get the go-ahead from wtc/whomever, I'll update the current patch to 
return PRInt32. I'll also review all the in-tree callers (basically fixing 
their handling of 'error'/'truncated buffer' situations).

Comment 9

[Malcolm Rowe]

Note: The fix just checked in for bug 162786 corrects the value returned when
truncation occurs to what it should be, given the existing implementation.

Comment 10

[Wan-Teh Chang]

Comment on attachment 146718 [details] [diff] [review]
Update PR_snprintf to ISO C99

We can't change the return type from PRUint32 to
PRInt32.

>+** PRUint32 ret = PR_snprintf(out, sizeof(out), ...);
>+** if (ret < 0)

ret < 0 needs to be replaced by ret == (PRUint32)-1
because ret is unsigned.

Comment 11

[Brendan Eich [:brendan]]

How can we change any NSPR entry point?  I thought they were all frozen, not
just type signatures but semantics, including bugs.

jsprf.c is for the JS engine only, and I believe all the engine's *snprintf
calls use big-enough buffers.  Counterexamples welcome, file bugs if you have
concrete cases (not just nagging doubts).

/be

Comment 12

[Malcolm Rowe]

(In reply to comment #10)
> We can't change the return type from PRUint32 to
> PRInt32.

Ok.

> >+** PRUint32 ret = PR_snprintf(out, sizeof(out), ...);
> >+** if (ret < 0)

I'll redo the patch to take that and bug 162786 into account (I'm assuming the
fix to the latter will generate a merge conflict).

(In reply to comment #11)
> How can we change any NSPR entry point?  I thought they were all frozen, not
> just type signatures but semantics, including bugs.

Well, that was really my question to wtc. That's obviously not strictly true, or
the patch to bug 162786 wouldn't have been checked in. There's currently no way
to detect a buffer overflow, and, prior to the recent patch, the return value
wasn't trustworthy anyway. I guess we need to balance updating the API to C99,
which will confuse developers less, on one hand, with leaving it alone, but with
no way to detect overflows, on the other.

> jsprf.c is for the JS engine only, and I believe all the engine's *snprintf
> calls use big-enough buffers.  Counterexamples welcome, file bugs if you have
> concrete cases (not just nagging doubts).

I doubt there are any problems. I only queried it because it seems to have been
autogenerated at some stage, and I wondered whether it needed to be kept in
sync. For example, will you be porting the patch from bug 162786?

Comment 13

[Brendan Eich [:brendan]]

jsprf.c is an old fork of prprf.c, to make JS standalone.  That happened after I
left the js group at Netscape to go found mozilla.org.  It's suboptimal, but not
a big enough deal that I've been motivated to fix it via some sed- or perl-based
scheme that might be too fragile to live with in the long run.  I can fix bug
162786 for sure (just did the patch, in fact).  Thanks for pointing that out.

/be

Comment 14

[tenthumbs]

How about creating a new function, say PR_snprintfC99, with the modern 
semantics and deprecating the old function?

Comment 15

[Malcolm Rowe]

Attachment: Update PR_snprintf to ISO C99, v2

I've updated the patch to fix the merge conflict, and correct the comment that
wtc pointed out. Otherwise, it's identical to the previous patch. Re-requesting
r/sr.

Comment 16

[Wan-Teh Chang]

I just read the description of the bug carefully.  This
is actually an enhancement request because it asks to
change the documented behavior of PR_snprintf regarding
whether output truncation should be considered an error.
(http://www.mozilla.org/projects/nspr/reference/html/prprf.html#23391)
Before the next NSPR major release (NSPR 5.0), this can
only be done by adding a new function.
 
I don't think returning the number of bytes that would
have been written is that useful.  Is there anything you
are working on that needs this information?  Does
PR_smprintf, which allocates the result buffer for you,
solve the problem for you?  I'd hate the clutter the NSPR
API just for the sake of emulating C99's snprintf.

Comment 17

[Malcolm Rowe]

(In reply to comment #16)
> I just read the description of the bug carefully.  This
> is actually an enhancement request because it asks to
> change the documented behavior of PR_snprintf regarding
> whether output truncation should be considered an error.

I agree that this is an enhancement request. However, PR_snprintf does not
currently treat truncation as an error, making it *impossible* to detect truncation.

> Before the next NSPR major release (NSPR 5.0), this can
> only be done by adding a new function.

Ok. I'm not sure that adding a 'PR_snprintfC99' function is useful enough to
warrant the clutter in the API, other than that it would clearly differentiate
the behaviour of the two functions.

> I don't think returning the number of bytes that would
> have been written is that useful.

I think the only problems here are that:
1. PR_snprintf attempts to follow the older snprintf implementations, but it
doesn't actually succeed (truncation is silent).
2. People will probably expect that PR_snprintf works like 'their' snprintf,
whether it's returning a signed value (q.v. /mailnews/mime/src/comi18n.cpp), or
returning the number of characters generated, per ISO C99.

As the original report said 'PR_snprintf should do one of these two things.'
(i.e., correctly implement what's intended by the documentation, or implement
ISO C99). I've gone down that latter route because I believed that it would be
more useful in the long run, but if we can't change the interface in this
fashion right now, it probably makes more sense to ensure that PR_snprintf
returns (PRUint32)-1 in the case of truncation, which is at least suggested by
the documentation.

Comments?

Comment 18

[tenthumbs]

I think it's obvious that, at some point in the past, NSPR chose to emulate
the C library snprintf function using the prevailing standard at the time. 
Furthermore, while NSPR could have named this function anything it wished 
(say "fred") it chose to mimic the standard name. I'm sure this was 
considered a design feature.

Well, time has passed and the standard has changed so the feature is now a
flaw. We know it causes confusion now. I'm sure the confusion will only get
worse. As I see it, the only question is how to fix it not if.  NSPR chose
mimickry once. If it does nothing then the mimickry is misleading and it 
perpetuates a bug.

Personally, I would follow the lead of the C standards committee and just 
change to the new standard. All those committees spent far more time on this 
then we have and I'm sure their decision is probably best. For people who 
really really want the old definition I think you could easily provide a 
compile-time option to restore the old behavior.

To me this is a bug fix but if you want to call it an enhancement that's 
fine with me.

Comment 19

[Wan-Teh Chang]

I agree that the current return value of PR_snprintf is
not the best design decision, but I think it is a
reasonable design decision.

Except for bug 162786 (which was fixed last week),
PR_snprintf correctly implements what's specified by its
documentation (both the comments in prprf.h and the HTML
documentation at
http://www.mozilla.org/projects/nspr/reference/html/prprf.html#23391).
Please let me know if I am wrong.

Again, I really need concrete examples of code that
needs to know the number of characters that would have
been written, and why that need can't be addressed by
the NSPR function PR_smprintf (note the 'm').

Comment 20

[Wan-Teh Chang]

Hi Malcolm, re: comment 17, could you tell me where
our documentation suggests that PR_snprintf should
return (PRUint32)-1 in the case of truncation?

Is it because you think truncation is an error?

Comment 21

[Malcolm Rowe]

Here's an example of a function that cannot use PR_smprintf, from the FTP
directory parser:

514               PR_snprintf(result->fe_size, sizeof(result->fe_size), 
515                           "%lld", fsz);

http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/ParseFTPList.cpp#514

Here, 'result' is a caller-provided pointer to a (caller-allocated) structure
with a fixed-size char[] array to hold the result. One could also imagine a
structure containing a filename, or possibly a stack-allocated filename, both of
which might have a platform-defined maximum size. PR_smprintf is also
significantly slower than PR_snprintf, since the storage is allocated on the heap.

Here's an example of code that would like to be able to detect result truncation:

6309     headersLen = PR_snprintf(p, l,"%s: %ld%s", ContentLenHeader, dataLen,
CRLFCRLF);
6310     if (headersLen == l) { // if PR_snprintf has ate all extra space
consider this as an error

http://lxr.mozilla.org/seamonkey/source/modules/plugin/base/src/nsPluginHostImpl.cpp#6309

> could you tell me where our documentation suggests that PR_snprintf
> should return (PRUint32)-1 in the case of truncation?
> Is it because you think truncation is an error?

The old BSD implementations match C99, i.e., they return the number of generated
characters. Older XPG implementations returned -1 for output truncation, I
believe, as did glibc <= 2.0.6 (current XPG and glibc >= 2.1 matches the C99
behaviour). I'm not aware of any implementations of snprintf that match NSPR's
policy of allowing silent output truncation.

Or, to summarise, a write to a buffer of size n returns the following value, x:

                  Ok           Truncated   Error
C99, BSD, etc     0 <= x < n   x >= n      x < 0
'old' glibc, etc  0 <= x < n   x < 0       x < 0
NSPR              0 <= x < n   x == n-1    x == (unsigned int)-1

One problem is that the documentation is silent on what exactly does constitute
an error - absent that, developers are quite understandably assuming that output
truncation would be an error, since the behaviour of PR_snprintf otherwise most
accurately matches the older implementations. Had you called the function
something else (PR_sprintf_safe, perhaps), this would not be a problem, but a
function called PR_snprintf is expected to behave like one of the two prevailing
snprintf implementations.

We should clarify the documentation regardless. However, for the implementation,
we have three choices:

1. Change PR_snprintf to match ISO C99. I think that this would be too drastic a
change, especially as a lot of code takes the return value of PR_snprintf and
passes it to Write() calls. (These need review anyway, since a failing return of
(PRUint32)-1 from PR_snprintf would undoubtably lead to a crash.
2. Change PR_snprintf to match XPG4, older glibc, et al. This is perhaps a less
risky option, though we would need to review callers to verify again that they
can cope acceptably with (PRUint32)-1 returns.
3. Do nothing and close this bug WONTFIX (after updating the documentation and
test suite to test for the current implementation). This might well be the best
option, but if we choose to provide a function called PR_snprintf that doesn't
really behave like any existing snprintf implementation, we had better improve
the documentation significantly.

Comment 22

[[e-mail]]

just as a note: if anyone changes the failure behavior of PR_snprintf, please
see bug 244478 (either the patch will need updating; or the consumer, if it's
been checked in).

Comment 23

[Darin Fisher]

> just as a note: if anyone changes the failure behavior of PR_snprintf, please
> see bug 244478 (either the patch will need updating; or the consumer, if it's
> been checked in).

this comment is evidence that changing the binary behavior of PR_snprintf is
going to lead to ABI incompatibilities.  we should be very careful.  i'd
recommend a new NSPR entry point over changing the current NSPR ABI.  backwards
compatibility is very important.  we cannot fix all consumers of NSPR :-(

we should of course fix the documentation for PR_snprintf to be accurate ;-)

Comment 24

[Darin Fisher]

we could use #ifdef to optionally map PR_snprintf to a better version, but the
PR_snprintf symbol in the resulting DSO should remain unchanged.

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

if we use #ifdef, source compatibility would still be broken; is that a concern?

Comment 26

[Darin Fisher]

> if we use #ifdef, source compatibility would still be broken; is that a concern?

source compat is less of a concern because people can opt into the new form of
the method or vice versa (whatever we decide).  ideally we'd make new code use
the new PR_snprintf symbol, but then we have to make sure that anyone compiling
against this new function understands the API change.  hmm... that could of
course be a problem :(

Comment 27

[Darin Fisher]

Comment on attachment 146868 [details] [diff] [review]
Update PR_snprintf to ISO C99, v2

removing sr request since 1) r=wtc is sufficient for NSPR, and 2) no point
asking for sr= without r= close in hand.

Comment 28

[Malcolm Rowe]

Unassigning, since I'm not going to have any time to work on this in the
immediate future.

From what I can see, we should:
1. Take attachment 146868 [details] [diff] [review] as a base.
2. Rename the existing PR_snprintf function to PR_snprintfC99, and create a new
PR_snprintf that is a wrapper for PR_snprintfC99, with different return values
(PRUint32 vs int, n-1 for truncation, (PRUint32)-1 for error).
3. Add an #ifdef in nsprpub/pr/include/prprf.h that conditionally #defines
PR_snprintf as PR_snprintfC99. I'd recommend that we also note that the #ifdef
will become default in NSPR 4.0 (though both entry points would remain).

Comment 29

[Malcolm Rowe]

Comment on attachment 146868 [details] [diff] [review]
Update PR_snprintf to ISO C99, v2

Also marking this patch obsolete and clearing review request, since it's clear
that this won't be going in as-is.

Comment 30

[Rich Megginson]

Another place where detecting overflow is essential is when trying to use fixed
size buffers as much as possible to avoid malloc, but needing to use a
non-truncated final value e.g.

char fixbuf[BUFSIZ];
char *buf = fixbuf;

if (PR_snprintf(buf, sizeof(fixbuf), "%s/%s/%s", foo, bar, baz) == (PRUint32)-1) {
   buf = PR_smprintf("%s/%s/%s", foo, bar, baz);
}
.... use buf ....
if (buf != fixbuf) {
   PR_smprintf_free(buf);
}

In this case, it doesn't matter if PR_snprintf returns the length as long as it
returns some value indicating overflow.

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.