I think I stumbled upon a bug in the i_snprintf() function. In the case of
vnsprintf() being available, it depends on vnsprintf() returning -1 when the
string was longer than the passed-in limit (or it won't terminate the
string.). But this isn't the C99-standardized behaviour, and newer glibc's
don't do that anymore either, so you can end up with a non-terminated
string. This patch should fix it, I think.
Index: strfuncs.c
===================================================================
RCS file: /home/cvs/dovecot/src/lib/strfuncs.c,v
retrieving revision 1.14
diff -c -u -r1.14 strfuncs.c
--- strfuncs.c 20 Oct 2002 03:19:10 -0000 1.14
+++ strfuncs.c 23 Oct 2002 11:19:39 -0000
@@ -401,7 +401,7 @@
va_end(args);
t_pop();
- if (ret < 0) {
+ if (ret < 0 || ret >= max_chars) {
str[max_chars-1] = '\0';
ret = strlen(str);
}
--
Thomas Wouters
On Wed, 2002-10-23 at 14:18, Thomas Wouters wrote:
I think I stumbled upon a bug in the i_snprintf() function. In the case of vnsprintf() being available, it depends on vnsprintf() returning -1 when the string was longer than the passed-in limit (or it won't terminate the string.). But this isn't the C99-standardized behaviour, and newer glibc's don't do that anymore either, so you can end up with a non-terminated string. This patch should fix it, I think.
Hm. vsnprintf() does terminate the string with \0 always, unless it returns -1. But I'll apply the patch anyway just to be sure :) Also my_vsyslog() didn't check the vsnprintf() return value at all. Have to go through that lib code more carefully some day..
On Wed, Oct 23, 2002 at 04:16:36PM +0300, Timo Sirainen wrote:
On Wed, 2002-10-23 at 14:18, Thomas Wouters wrote:
I think I stumbled upon a bug in the i_snprintf() function. In the case of vnsprintf() being available, it depends on vnsprintf() returning -1 when the string was longer than the passed-in limit (or it won't terminate the string.). But this isn't the C99-standardized behaviour, and newer glibc's don't do that anymore either, so you can end up with a non-terminated string. This patch should fix it, I think.
Hm. vsnprintf() does terminate the string with \0 always, unless it returns -1. But I'll apply the patch anyway just to be sure :)
Hmm. Yeah, testing confirms you are right, I was just confused by the Linux manpage on the printf-functions. The FreeBSD manpage states it unambigiously:
Snprintf() and vsnprintf() will write at most size-1 of the characters
printed into the output string (the size'th character then gets the
terminating `\0')
Hey, that means I can go back and clean up some of my old code :-P
-- Thomas Wouters thomas@xs4all.net
Hi! I'm a .signature virus! copy me into your .signature file to help me spread!
- On 2002.10.23, in 20021023111843.GA3563@xs4all.nl,
- "Thomas Wouters" thomas@xs4all.net wrote:
I think I stumbled upon a bug in the i_snprintf() function. In the case of vnsprintf() being available, it depends on vnsprintf() returning -1 when the string was longer than the passed-in limit (or it won't terminate the string.). But this isn't the C99-standardized behaviour, and newer glibc's don't do that anymore either, so you can end up with a non-terminated string. This patch should fix it, I think.
Ah, I'm glad glibc has fixed this. (Have they fixed snprintf(), too?) This patch looks correct for Solaris, as well.
There's an alternative approach that depends on the newer vsnprintf() behavior. You can use
{ char c, *buf; length = vsnprintf(&c, 1, fmt, vp); buf = malloc(length+1); vsnprintf(buf, length, fmt, vp); }
to dynamically size the buffer as large as it needs to be. For general formatting routines, I sometimes use this idiom (not totally valid C, and no error-checking):
format(...) { static char *buf = NULL; /* workspace */ static int buf_len = 0; /* usable area in buf (size - '\0') */ int fmt_len; /* length of formatted string */ ...
if (buf == NULL) {
/* init to minimum size */
buf = malloc(128+1);
buf_len = 128;
}
/* Format once, and find out how long the formatted data was */
fmt_len = vsnprintf(buf, buf_len+1, fmt, vp);
/* If longer than buf, resize buf amnd try again */
if (fmt_len > buf_len) {
realloc(buf, (buf_len = fmt_len) + 1);
vsnprintf(buf, buf_len+1, fmt, vp);
}
...
}
This makes sure I always have a big-enough buffer, without lots of unneeded mallocing and freeing. The down-side is that it only works on modern systems, because of the issue with [v]snprintf()'s return value. And it sometimes formats the string twice, which is perhaps not good, but perhaps alright. And I guess it's not re-entrant, which could be a problem, though it could be made re-entrant with some work. :)
Anyway, this is perhaps a little much for a first post to a list from someone who hasn't found time to look at or compile the code yet, but off it goes....
-- -D. We establised a fine coffee. What everybody can say Sun Project, APC/UCCO TASTY! It's fresh, so-mild, with some special coffee's University of Chicago bitter and sourtaste. "LET'S HAVE SUCH A COFFEE! NOW!" dgc@uchicago.edu Please love CAFE MIAMI. Many thanks.
On Wed, 2002-10-23 at 20:13, David Champion wrote:
There's an alternative approach that depends on the newer vsnprintf() behavior. You can use
{ char c, *buf; length = vsnprintf(&c, 1, fmt, vp); buf = malloc(length+1); vsnprintf(buf, length, fmt, vp); }
to dynamically size the buffer as large as it needs to be. For general formatting routines, I sometimes use this idiom (not totally valid C, and no error-checking):
Not too bad idea :) Dovecot currently uses a bit modified GLIB's g_printf_string_upper_bound() and then allocates enough memory based on it. Maybe using vsnprintf() could be optional (detected by configure if it works), if it's better/faster than my upper_bound() function.
participants (3)
-
David Champion
-
Thomas Wouters
-
Timo Sirainen