Open
Bug 229662
Opened 21 years ago
Updated 1 year ago
PR_snprintf does not return a useful error value
Categories
(NSPR :: NSPR, defect)
NSPR
NSPR
Tracking
(Not tracked)
NEW
People
(Reporter: tenthumbs, Unassigned)
References
Details
Attachments
(2 obsolete files)
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.
see also bug 162786
Comment 2•20 years ago
|
||
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).
Updated•20 years ago
|
Assignee: wchang0222 → malcolm-bmo
Status: NEW → ASSIGNED
Comment 3•20 years ago
|
||
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.
Attachment #146718 -
Flags: review?(wchang0222)
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.
Updated•20 years ago
|
Attachment #146718 -
Flags: superreview?(darin)
Comment 5•20 years ago
|
||
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•20 years ago
|
||
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.
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•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
(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•20 years ago
|
||
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
Reporter | ||
Comment 14•20 years ago
|
||
How about creating a new function, say PR_snprintfC99, with the modern semantics and deprecating the old function?
Comment 15•20 years ago
|
||
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.
Attachment #146718 -
Attachment is obsolete: true
Updated•20 years ago
|
Attachment #146868 -
Flags: superreview?(darin)
Attachment #146868 -
Flags: review?(wchang0222)
Updated•20 years ago
|
Attachment #146718 -
Flags: superreview?(darin)
Attachment #146718 -
Flags: review?(wchang0222)
Comment 16•20 years ago
|
||
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•20 years ago
|
||
(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?
Reporter | ||
Comment 18•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
> 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•20 years ago
|
||
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•20 years ago
|
||
if we use #ifdef, source compatibility would still be broken; is that a concern?
Comment 26•20 years ago
|
||
> 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•20 years ago
|
||
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.
Attachment #146868 -
Flags: superreview?(darin)
Comment 28•20 years ago
|
||
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).
Assignee: malcolm-bmo → wchang0222
Status: ASSIGNED → NEW
Comment 29•20 years ago
|
||
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.
Attachment #146868 -
Attachment is obsolete: true
Attachment #146868 -
Flags: review?(wchang0222)
Comment 30•19 years ago
|
||
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.
Updated•18 years ago
|
QA Contact: wtchang → nspr
Updated•16 years ago
|
Flags: wanted1.9.0.x?
OS: Linux → All
Hardware: PC → All
Updated•16 years ago
|
Flags: wanted1.9.0.x? → wanted1.9.0.x-
Updated•2 years ago
|
Severity: normal → S3
Comment 31•1 year ago
|
||
The bug assignee is inactive on Bugzilla, so the assignee is being reset.
Assignee: wtc → nobody
You need to log in
before you can comment on or make changes to this bug.
Description
•