Full_Name: Hallvard B Furuseth Version: HEAD OS: URL: Submission from: (NULL) (129.240.186.42) Submitted by: hallvard OpenLDAP is full of code like ptr += snprintf( ptr, ... ); and bv.bv_len = snprintf( ... ); <use bv>; However snprintf does not return the number of characters written, it returns the number of characters it would write if no truncation occurs, not including the terminating '\0'. Correct use is: len = snprintf( buf, buflen, ... ); if ( len >= buflen ) { Truncated to (buflen-1) chars + '\0' - or no change if buflen == 0; } I imagine we could use some helper functions here, e.g. a snprintf_long() for output of just a single arg, but I won't dive into that now. The snprintf calls that just output a %s can probably just as well be replaced with something else.
On Sun, 2006-04-02 at 15:01 +0000, h.b.furuseth@usit.uio.no wrote: > Full_Name: Hallvard B Furuseth > Version: HEAD > OS: > URL: > Submission from: (NULL) (129.240.186.42) > Submitted by: hallvard > > > OpenLDAP is full of code like > ptr += snprintf( ptr, ... ); > and > bv.bv_len = snprintf( ... ); > <use bv>; > > However snprintf does not return the number of characters written, it > returns the number of characters it would write if no truncation occurs, > not including the terminating '\0'. In many cases, especially when used to compute the length of a berval, snprintf is used under the assumption the buffer is large enough to contain the formatted output, based on the knowledge of the value that is about to be printed. For example, when used to format integers, the buffer is usually created as char buf[] = "18446744073709551615UL"; which is the string representation of ULONG_MAX, or anything like that. In general, I concur a more consistent and reliable usage of that function should occur throughout the code. p. Ing. Pierangelo Masarati Responsabile Open Solution OpenLDAP Core Team SysNet s.n.c. Via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it ------------------------------------------ Office: +39.02.23998309 Mobile: +39.333.4963172 Email: pierangelo.masarati@sys-net.it ------------------------------------------
Pierangelo Masarati writes: > In many cases, especially when used to compute the length of a berval, > snprintf is used under the assumption the buffer is large enough to > contain the formatted output, based on the knowledge of the value that > is about to be printed. For example, when used to format integers, > the buffer is usually created as > > char buf[] = "18446744073709551615UL"; > > which is the string representation of ULONG_MAX, or anything like that. Ah, so that's what these weird string constants are for. That's wrong too - whether or not that's big enough for ULONG_MAX depends on the width of unsigned long. We can use something like this. Not sure if ldap_pvt or some other file is the best place for the macro: ldap_pvt.h: #include <limits.h> /* Buffer space for sign, decimal digits and \0. Note: log10(2) < 146/485. */ #define LDAP_PVT_INTTYPE_CHARS(type) (((sizeof(type)*CHAR_BIT-1)*146)/485 + 3) (Did I post that before? Or was that to some other project?) whatever.c: char buf[LDAP_PVT_INTTYPE_CHARS(unsigned long)]; Also, I think any remaining 'char buf[] = "unused text"; code should be changed to char buf[sizeof("whatever")]; so that one can tell from reading the code that the initial contents is irrelevant. (As long as it's irrelevant even at failure :-) I've been staring at some of that code and wondered WTF was going on. -- Hallvard
moved from Incoming to Software Bugs
h.b.furuseth@usit.uio.no wrote: > OpenLDAP is full of code like > ptr += snprintf( ptr, ... ); > and > bv.bv_len = snprintf( ... ); > <use bv>; Let me add that there seem to be occasional improper uses of sprintf() too (usually in marginal code, fortunately). p. Ing. Pierangelo Masarati OpenLDAP Core Team SysNet s.r.l. via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it --------------------------------------- Office: +39 02 23998309 Mobile: +39 333 4963172 Email: pierangelo.masarati@sys-net.it ---------------------------------------
changed notes changed state Open to Suspended
changed state Suspended to Open
changed notes
changed notes changed state Open to Test
changed notes changed state Test to Suspended
moved from Software Bugs to Development
Some fixes in HEAD Some fixes in RE24 (Suspend instead of Close when released, there are lots left)