test_builtin failing? or just 64-bit platforms
Anyone else getting test_builtin failures with current CVS, or does it only show on 64-bit platforms? Changes in the past week seem to have caused the failure. Isolated to following (will post bug on SF): ***** older CVS works ***** 201 mark@gonzo.per.dem.csiro.au python Python 2.2b2+ (#539, Nov 26 2001, 09:52:25) [C] on osf1V4 Type "help", "copyright", "credits" or "license" for more information.
***** current CVS fails ***** 202 mark@gonzo.per.dem.csiro.au cd dist/src 203 mark@gonzo.per.dem.csiro.au ./python Python 2.2b2+ (#541, Dec 1 2001, 08:04:58) [C] on osf1V4 Type "help", "copyright", "credits" or "license" for more information.
-- Mark Favas - m.favas@per.dem.csiro.au CSIRO, Private Bag No 5, Wembley, Western Australia 6913, AUSTRALIA
[Mark Favas]
Anyone else getting test_builtin failures with current CVS,
No.
Thanks!
It doesn't make sense, but it *smells* like a sprintf -> PyOS_snprintf screwup ... OK, our int repr code has always been wrong(!): static PyObject * int_repr(PyIntObject *v) { char buf[20]; PyOS_snprintf(buf, sizeof(buf), "%ld", v->ob_ival); return PyString_FromString(buf); } 20 bytes isn't enough to hold the result on a 64-bit box (insert rant about the idiot practice of trying to make stack buffers as small as possible). You have 20 characters in your result, but need 21 to hold the trailing 0 byte too. I don't know what snprintf does when there's not enough room, but I think you just showed us what it does on Tru64 <wink>. Doing repr impllicitly instead works because that goes thru int_print instead of int_repr. Doing repr explicitly "worked" before by accident (the trailing null overwrite some random byte on the stack). Please change 20 to 200(*) (or 64 -- your choice) and see whether that fixes it?
I don't know what snprintf does when there's not enough room, but I think you just showed us what it does on Tru64 <wink>.
Hm, snprintf is *supposed* to truncate the result, but it seems that here it refused to do anything. Maybe PyOS_snprintf should be a wrapper that checks the return value of snprintf? I noticed that *none* of the recently checked-in PyOS_snprintf calls have their return value checked, and I think it's better to leave it that way (since in many cases we really don't know what to do instead) -- maybe PyOS_snprintf should even return void (or does it already?). --Guido van Rossum (home page: http://www.python.org/~guido/)
Hm, snprintf is *supposed* to truncate the result,
According to C99, yes, but MS has its own pre-C99 snprintf semantics, and glibc has had at least two different versions.
but it seems that here it refused to do anything. Maybe PyOS_snprintf should be a wrapper that checks the return value of snprintf?
Check it for what? We can't (at least not yet) count on uniform behavior across platform snprintf implementations.
I noticed that *none* of the recently checked-in PyOS_snprintf calls have their return value checked,
They were all derived (and most mindlessly) from existing sprintf calls that didn't check either. A goal of this transformation was to change as little as possible.
The comments suggest it wants to return the C99-defined value (an int, < 0 for an encoding error, else the number of characters (excluding \0) written if the size is big enough, else the number of characters that would have been written (excluding \0) had size been big enough. So the output was converted in full iff the return value is non-negative and strictly less than the size passed in. That's fine by me, and I'll note that MS snprintf meets that much too (if size isn't big enough, it returns a negative result). So perhaps in a debug build PyOS_snprintf could assert that the returned value is non-negative and less than the passed-in size ...
See below. On my system, the snprintf man page documents both behaviors, with the gcc version where it switches.
Should work for gcc too. Note that the Tru64 output suggests that the behavior (== effect on the buffer) may differ too: a truncated version of the desired output may or may not be written to the buffer. This can be addressed by pre-filling the buffer with a useful pattern in PyOS_snprintf before calling the system's snprintf -- e.g. "*-*-*-*" (to give a patriotic example :-). --Guido van Rossum (home page: http://www.python.org/~guido/) PS: I feel like I'm writing this on borrowed time -- Comcast and Excite@Home haven't disconnected me from the net yet. :-)
Oops, you're right. It seems that mysnprintf.c doesn't copy any bytes to the buffer when there's an overflow. --Guido van Rossum (home page: http://www.python.org/~guido/)
Oops, you're right. It seems that mysnprintf.c doesn't copy any bytes to the buffer when there's an overflow.
Confirmed, and fixed in CVS. --Guido van Rossum (home page: http://www.python.org/~guido/)
[Guido]
Which later turned out to be our fault. However, the MS native _snprintf is worse than that: if the buffer isn't big enough, it fills it to the end *without* a trailing \0. A subsequent (e.g.) PyString_FromString() would then pick up an arbitrarily long stretch of garbage bytes beyond the buffer's end. You can see the effect by, e.g., making int_repr's buffer 4 bytes, then doing repr(sys.maxint), on Windows.
I don't know why you'd want to burn time doing this -- IMO, if the buffer isn't big enough, it should almost always be due to an internal Python bug. The point of PyOS_snprintf isn't to let us be lazy about buffer decls or careless with formats, it's to prevent exploits even when we screw up.
PS: I feel like I'm writing this on borrowed time -- Comcast and Excite@Home haven't disconnected me from the net yet. :-)
It's quite the soap opera! AT&T pulled out of the talks, and AT&T's @Home customers were then cut off. The other partners are still negotiating. Moral of the story: if you ever start a moderately successful ISP, think twice before spending 6 billion dollars to acquire a portal. If you can't resist, at least avoid selling your services below cost to companies much bigger than you, counting on portal advertising dollars to make up the difference. if-it-cost-6-billion-it-must-be-worth-at-least-12-ly y'rs - tim
The uses of PyOS_snprintf() in sysmodule.c actually checks the return value. It always checked the sprintf() returned value and triggered a Py_FatalError() if the return value was too big. I changed it to print a warning message that the output was truncated. Jeremy
[Jeremy Hylton]
The test appeared off on both ends to me, so I fiddled this code. Please check the new version against your vision of sanity (my vision: there's nothing special about -1: any return value < 0 is a problem; and there's also a problem if the return value equals the buffer size, since the return value is exclusive of the \0 byte).
"TP" == Tim Peters <tim.one@home.com> writes:
TP> [Jeremy Hylton]
TP> The test appeared off on both ends to me, so I fiddled this TP> code. Please check the new version against your vision of TP> sanity (my vision: there's nothing special about -1: any return TP> value < 0 is a problem; and there's also a problem if the return TP> value equals the buffer size, since the return value is TP> exclusive of the \0 byte). Definitely correct on the latter. Sensibly conservative on the former. The Linux man pages are silent on negative return values other than -1. Thanks for the fixes. Jeremy
[Jeremy]
My draft copy of C99 sez: 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. -1 isn't mentioned at all. What does the final C99 say? Regardless, MS's pre-C99 implementation says: _snprintf returns the number of bytes stored in buffer, not counting the terminating null character. If the number of bytes required to store the data exceeds count, then count bytes of data are stored in buffer and a negative value is returned. So -1 sounds Linux-specific.
Thanks for the fixes.
Thank *you* for the opportunity to be thorough <wink>.
Being a Unix old-timer, I cringe every time I see someone test for errors using "== -1". Except for anomalies like getpid() (which in the olden days could return any negative value except -1 as a legitimate pid), for the caller *any* negative value should be interpreted as an error, and tested for with "< 0". Never mind that most functions return -1. -1000 is just as much an error. Unhelpfully, --Guido van Rossum (home page: http://www.python.org/~guido/)
"TP" == Tim Peters <tim.one@home.com> writes:
TP> It doesn't make sense, but it *smells* like a sprintf -> TP> PyOS_snprintf screwup ... OK, our int repr code has always TP> been wrong(!): | static PyObject * | int_repr(PyIntObject *v) | { | char buf[20]; | PyOS_snprintf(buf, sizeof(buf), "%ld", v->ob_ival); | return PyString_FromString(buf); | } TP> 20 bytes isn't enough to hold the result on a 64-bit box TP> (insert rant about the idiot practice of trying to make stack TP> buffers as small as possible). You have 20 characters in your TP> result, but need 21 to hold the trailing 0 byte too. I don't TP> know what snprintf does when there's not enough room, but I TP> think you just showed us what it does on Tru64 <wink>. Heh, I was going to suggest that this might be a good place to substitute a call to PyString_FromFormat*() but then I read this little nugget: case 'd': case 'i': case 'x': (void) va_arg(count, int); /* 20 bytes should be enough to hold a 64-bit integer */ n += 20; break; love-ly y'rs, -Barry
[Barry]
This use of 20 is fine (I checked all occurrences of "20" in the codebase, BTW); int_repr's use of 20 was wrong because it failed to allow for the trailing \0 byte too; the loop in PyString_FromFormatV doesn't have to worry about that (PyString_FromStringAndSize() later adds 1 of its own for the trailing \0 -- of course it doesn't actually add anything, but it adds 1 "in effect" <wink -- jeez, C string handling is obscure!>).
[Mark Favas]
Anyone else getting test_builtin failures with current CVS,
No.
Thanks!
It doesn't make sense, but it *smells* like a sprintf -> PyOS_snprintf screwup ... OK, our int repr code has always been wrong(!): static PyObject * int_repr(PyIntObject *v) { char buf[20]; PyOS_snprintf(buf, sizeof(buf), "%ld", v->ob_ival); return PyString_FromString(buf); } 20 bytes isn't enough to hold the result on a 64-bit box (insert rant about the idiot practice of trying to make stack buffers as small as possible). You have 20 characters in your result, but need 21 to hold the trailing 0 byte too. I don't know what snprintf does when there's not enough room, but I think you just showed us what it does on Tru64 <wink>. Doing repr impllicitly instead works because that goes thru int_print instead of int_repr. Doing repr explicitly "worked" before by accident (the trailing null overwrite some random byte on the stack). Please change 20 to 200(*) (or 64 -- your choice) and see whether that fixes it?
I don't know what snprintf does when there's not enough room, but I think you just showed us what it does on Tru64 <wink>.
Hm, snprintf is *supposed* to truncate the result, but it seems that here it refused to do anything. Maybe PyOS_snprintf should be a wrapper that checks the return value of snprintf? I noticed that *none* of the recently checked-in PyOS_snprintf calls have their return value checked, and I think it's better to leave it that way (since in many cases we really don't know what to do instead) -- maybe PyOS_snprintf should even return void (or does it already?). --Guido van Rossum (home page: http://www.python.org/~guido/)
Hm, snprintf is *supposed* to truncate the result,
According to C99, yes, but MS has its own pre-C99 snprintf semantics, and glibc has had at least two different versions.
but it seems that here it refused to do anything. Maybe PyOS_snprintf should be a wrapper that checks the return value of snprintf?
Check it for what? We can't (at least not yet) count on uniform behavior across platform snprintf implementations.
I noticed that *none* of the recently checked-in PyOS_snprintf calls have their return value checked,
They were all derived (and most mindlessly) from existing sprintf calls that didn't check either. A goal of this transformation was to change as little as possible.
The comments suggest it wants to return the C99-defined value (an int, < 0 for an encoding error, else the number of characters (excluding \0) written if the size is big enough, else the number of characters that would have been written (excluding \0) had size been big enough. So the output was converted in full iff the return value is non-negative and strictly less than the size passed in. That's fine by me, and I'll note that MS snprintf meets that much too (if size isn't big enough, it returns a negative result). So perhaps in a debug build PyOS_snprintf could assert that the returned value is non-negative and less than the passed-in size ...
See below. On my system, the snprintf man page documents both behaviors, with the gcc version where it switches.
Should work for gcc too. Note that the Tru64 output suggests that the behavior (== effect on the buffer) may differ too: a truncated version of the desired output may or may not be written to the buffer. This can be addressed by pre-filling the buffer with a useful pattern in PyOS_snprintf before calling the system's snprintf -- e.g. "*-*-*-*" (to give a patriotic example :-). --Guido van Rossum (home page: http://www.python.org/~guido/) PS: I feel like I'm writing this on borrowed time -- Comcast and Excite@Home haven't disconnected me from the net yet. :-)
Oops, you're right. It seems that mysnprintf.c doesn't copy any bytes to the buffer when there's an overflow. --Guido van Rossum (home page: http://www.python.org/~guido/)
Oops, you're right. It seems that mysnprintf.c doesn't copy any bytes to the buffer when there's an overflow.
Confirmed, and fixed in CVS. --Guido van Rossum (home page: http://www.python.org/~guido/)
[Guido]
Which later turned out to be our fault. However, the MS native _snprintf is worse than that: if the buffer isn't big enough, it fills it to the end *without* a trailing \0. A subsequent (e.g.) PyString_FromString() would then pick up an arbitrarily long stretch of garbage bytes beyond the buffer's end. You can see the effect by, e.g., making int_repr's buffer 4 bytes, then doing repr(sys.maxint), on Windows.
I don't know why you'd want to burn time doing this -- IMO, if the buffer isn't big enough, it should almost always be due to an internal Python bug. The point of PyOS_snprintf isn't to let us be lazy about buffer decls or careless with formats, it's to prevent exploits even when we screw up.
PS: I feel like I'm writing this on borrowed time -- Comcast and Excite@Home haven't disconnected me from the net yet. :-)
It's quite the soap opera! AT&T pulled out of the talks, and AT&T's @Home customers were then cut off. The other partners are still negotiating. Moral of the story: if you ever start a moderately successful ISP, think twice before spending 6 billion dollars to acquire a portal. If you can't resist, at least avoid selling your services below cost to companies much bigger than you, counting on portal advertising dollars to make up the difference. if-it-cost-6-billion-it-must-be-worth-at-least-12-ly y'rs - tim
The uses of PyOS_snprintf() in sysmodule.c actually checks the return value. It always checked the sprintf() returned value and triggered a Py_FatalError() if the return value was too big. I changed it to print a warning message that the output was truncated. Jeremy
[Jeremy Hylton]
The test appeared off on both ends to me, so I fiddled this code. Please check the new version against your vision of sanity (my vision: there's nothing special about -1: any return value < 0 is a problem; and there's also a problem if the return value equals the buffer size, since the return value is exclusive of the \0 byte).
"TP" == Tim Peters <tim.one@home.com> writes:
TP> [Jeremy Hylton]
TP> The test appeared off on both ends to me, so I fiddled this TP> code. Please check the new version against your vision of TP> sanity (my vision: there's nothing special about -1: any return TP> value < 0 is a problem; and there's also a problem if the return TP> value equals the buffer size, since the return value is TP> exclusive of the \0 byte). Definitely correct on the latter. Sensibly conservative on the former. The Linux man pages are silent on negative return values other than -1. Thanks for the fixes. Jeremy
[Jeremy]
My draft copy of C99 sez: 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. -1 isn't mentioned at all. What does the final C99 say? Regardless, MS's pre-C99 implementation says: _snprintf returns the number of bytes stored in buffer, not counting the terminating null character. If the number of bytes required to store the data exceeds count, then count bytes of data are stored in buffer and a negative value is returned. So -1 sounds Linux-specific.
Thanks for the fixes.
Thank *you* for the opportunity to be thorough <wink>.
Being a Unix old-timer, I cringe every time I see someone test for errors using "== -1". Except for anomalies like getpid() (which in the olden days could return any negative value except -1 as a legitimate pid), for the caller *any* negative value should be interpreted as an error, and tested for with "< 0". Never mind that most functions return -1. -1000 is just as much an error. Unhelpfully, --Guido van Rossum (home page: http://www.python.org/~guido/)
"TP" == Tim Peters <tim.one@home.com> writes:
TP> It doesn't make sense, but it *smells* like a sprintf -> TP> PyOS_snprintf screwup ... OK, our int repr code has always TP> been wrong(!): | static PyObject * | int_repr(PyIntObject *v) | { | char buf[20]; | PyOS_snprintf(buf, sizeof(buf), "%ld", v->ob_ival); | return PyString_FromString(buf); | } TP> 20 bytes isn't enough to hold the result on a 64-bit box TP> (insert rant about the idiot practice of trying to make stack TP> buffers as small as possible). You have 20 characters in your TP> result, but need 21 to hold the trailing 0 byte too. I don't TP> know what snprintf does when there's not enough room, but I TP> think you just showed us what it does on Tru64 <wink>. Heh, I was going to suggest that this might be a good place to substitute a call to PyString_FromFormat*() but then I read this little nugget: case 'd': case 'i': case 'x': (void) va_arg(count, int); /* 20 bytes should be enough to hold a 64-bit integer */ n += 20; break; love-ly y'rs, -Barry
[Barry]
This use of 20 is fine (I checked all occurrences of "20" in the codebase, BTW); int_repr's use of 20 was wrong because it failed to allow for the trailing \0 byte too; the loop in PyString_FromFormatV doesn't have to worry about that (PyString_FromStringAndSize() later adds 1 of its own for the trailing \0 -- of course it doesn't actually add anything, but it adds 1 "in effect" <wink -- jeez, C string handling is obscure!>).
participants (7)
-
barry＠zope.com -
Fredrik Lundh -
Guido van Rossum -
Jeremy Hylton -
jeremy＠zope.com
-
Mark Favas -
Tim Peters