"Ronald S. Bultje" <[email protected]> writes: > Hi, > > On Thu, Aug 2, 2012 at 12:29 PM, Måns Rullgård <[email protected]> wrote: >> "Ronald S. Bultje" <[email protected]> writes: >> >>> Hi, >>> >>> On Wed, Aug 1, 2012 at 11:58 AM, Måns Rullgård <[email protected]> wrote: >>>> Luca Barbato <[email protected]> writes: >>>> >>>>> +#if !HAVE_SNPRINTF >>>>> +#ifdef _MSC_VER >>>>> +#define vsnprintf _vsnprintf >>>>> +#endif >>>>> + >>>>> +int snprintf(char *buffer, size_t bufsize, const char *fmt, ...) >>>>> +{ >>>>> + va_list ap; >>>>> + int ret; >>>>> + >>>>> + if ((int)bufsize <= 0) return -1; >>>>> + va_start(ap, fmt); >>>>> + ret = vsnprintf(buffer, bufsize-1, fmt, ap); >>>>> + if (ret < 0) { >>>>> + buffer[bufsize - 1] = '\0'; >>>>> + ret = bufsize - 1; >>>>> + } >>>>> + va_end(ap); >>>>> + return ret; >>>>> +} >>>> >>>> This assumes that any system without snprintf will have vsnprintf with >>>> microsoft semantics. That's a pretty bold assumption. >>> >>> As for vsnprintf behaviour, from MSDN: >>> "vsnprintf, _vsnprintf, and _vsnwprintf return the number of >>> characters written if the number of characters to write is less than >>> or equal to count; if the number of characters to write is greater >>> than count, these functions return -1 indicating that output has been >>> truncated. The return value does not include the terminating null, if >>> one is written." >>> >>> So if format, buffer != NULL and count > 0, a return value of -1 means >>> that the buffer was overrun and is filled with data ("truncated"), so >>> writing a '\0' at the end and returning count should be OK then. >> >> What if something else goes wrong causing it to return -1? The >> documentation is a bit sketchy on failure modes. >> >> The safest solution is probably to zero-fill the whole buffer before >> calling the _vsnprintf() with a size one less than the actual buffer >> size. >> >>> As you pointed out, if the return value equals count, no zero was >>> written and we should add that. >>> >>> Do we care that the return value of this snprintf() drop-in will not >>> be the amount of characters that would have been written, but is >>> simply count, if the output was truncated? >> >> I don't think we currently rely on that behaviour anywhere, but it's >> always risky to deviate from the standard. > > My manpage says:
Never trust man pages. Read the official spec. > " The snprintf() and vsnprintf() functions will write at most n-1 > of the characters printed into the output string (the n'th character > then gets the > terminating `\0'); if the return value is greater than or equal > to the n argument, the string was too short and some of the printed > characters were > discarded. The output is always null-terminated." This happens to be equivalent to what the spec says. > I.e. returning 'n' if the string is too short (regardless of whether > the string would've been longer if the buffer was big enough) should > be OK. I believe we actually rely on this, You believe incorrectly. Certainly, the glibc snprintf() does not behave that way. > I see us using this type of construct a lot in e.g. avconv.c: > > len += snprintf(args + len, sizeof(args) - len, "sample_fmts=%s:", > len += snprintf(args + len, sizeof(args) - len, > "sample_rates=%s:", > len += snprintf(args + len, sizeof(args) - len, > "channel_layouts=%s:", > > if we returned len > sizeof(string), string+len would point outside > the buffer. Isn't that undefined? Perhaps the code is broken and needs to be fixed. Without looking at the code in question more carefully, I can't say whether or not such an overflow is possible. For example, the string arguments could be taken from a fixed list and the buffer sized to accomodate the longest possible combination, or there could be some validation prior to those snprintf() calls. If neither of those is true, the code is likely broken and in need of fixing. -- Måns Rullgård [email protected] _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
