[U-Boot] [PATCH 0/4] Buffer overruns in printf
The printf family of functions in U-Boot cannot deal with a situation where the caller provides a buffer which turns out to be too small for the format string. This can result in buffer overflows, stack overflows and other bad behavior.
This patch series tidies this up in the common vsprintf.c code, and also some network code (as an example of where this might lead).
Simon Glass (2): Add limits.h to hold basic limits Use snprintf() in network code
Sonny Rao (2): Add safe vsnprintf and snprintf library functions Make printf and vprintf safe from buffer overruns
common/console.c | 10 +- fs/ubifs/ubifs.h | 4 +- include/common.h | 8 ++- include/limits.h | 40 +++++++ lib/vsprintf.c | 316 ++++++++++++++++++++++++++++++++++++++++++------------ net/eth.c | 10 ++- net/net.c | 15 ++- net/nfs.c | 3 +- net/tftp.c | 3 +- 9 files changed, 323 insertions(+), 86 deletions(-) create mode 100644 include/limits.h
This brings a basic limits.h implementation into U-Boot.
Signed-off-by: Simon Glass sjg@chromium.org --- fs/ubifs/ubifs.h | 4 +--- include/limits.h | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 include/limits.h
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h index 0af471a..e7b6e43 100644 --- a/fs/ubifs/ubifs.h +++ b/fs/ubifs/ubifs.h @@ -311,9 +311,7 @@ struct file { #define MAX_LFS_FILESIZE 0x7fffffffffffffffUL #endif
-#define INT_MAX ((int)(~0U>>1)) -#define INT_MIN (-INT_MAX - 1) -#define LLONG_MAX ((long long)(~0ULL>>1)) +#include <limits.h>
/* * These are the fs-independent mount-flags: up to 32 flags are supported diff --git a/include/limits.h b/include/limits.h new file mode 100644 index 0000000..1021291 --- /dev/null +++ b/include/limits.h @@ -0,0 +1,40 @@ +/* + * Copyright (c) 2011 The Chromium OS Authors. + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#ifndef __LIMITS_H +#define __LIMITS_H + +/* Keep all our limits in one place */ + +#define USHRT_MAX ((u16)(~0U)) +#define SHRT_MAX ((s16)(USHRT_MAX>>1)) +#define SHRT_MIN ((s16)(-SHRT_MAX - 1)) +#define INT_MAX ((int)(~0U>>1)) +#define INT_MIN (-INT_MAX - 1) +#define UINT_MAX (~0U) +#define LONG_MAX ((long)(~0UL>>1)) +#define LONG_MIN (-LONG_MAX - 1) +#define ULONG_MAX (~0UL) +#define LLONG_MAX ((long long)(~0ULL>>1)) +#define LLONG_MIN (-LLONG_MAX - 1) +#define ULLONG_MAX (~0ULL) + +#endif
From: Sonny Rao sonnyrao@chromium.org
From: Sonny Rao sonnyrao@chromium.org
These functions are useful in U-Boot because they allow a graceful failure rather than an unpredictable stack overflow when printf() buffers are exceeded.
Mostly copied from the Linux kernel. I copied vscnprintf and scnprintf so we can change printf and vprintf to use the safe implementation but still return the correct values.
Signed-off-by: Simon Glass sjg@chromium.org --- include/common.h | 8 ++- lib/vsprintf.c | 316 ++++++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 256 insertions(+), 68 deletions(-)
diff --git a/include/common.h b/include/common.h index d244bd4..fbcc55f 100644 --- a/include/common.h +++ b/include/common.h @@ -682,9 +682,15 @@ unsigned long long simple_strtoull(const char *cp,char **endp,unsigned int base) long simple_strtol(const char *cp,char **endp,unsigned int base); void panic(const char *fmt, ...) __attribute__ ((format (__printf__, 1, 2), noreturn)); -int sprintf(char * buf, const char *fmt, ...) +int sprintf(char *buf, const char *fmt, ...) __attribute__ ((format (__printf__, 2, 3))); +int snprintf(char *buf, size_t size, const char *fmt, ...) + __attribute__ ((format (__printf__, 3, 4))); +int scnprintf(char *buf, size_t size, const char *fmt, ...) + __attribute__ ((format (__printf__, 3, 4))); int vsprintf(char *buf, const char *fmt, va_list args); +int vsnprintf(char *buf, size_t size, const char *fmt, va_list args); +int vscnprintf(char *buf, size_t size, const char *fmt, va_list args);
/* lib/strmhz.c */ char * strmhz(char *buf, unsigned long hz); diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 79dead3..bac6f30 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -16,6 +16,7 @@ #include <errno.h>
#include <common.h> +#include <limits.h> #if !defined (CONFIG_PANIC_HANG) #include <command.h> #endif @@ -289,7 +290,8 @@ static noinline char* put_dec(char *buf, unsigned NUM_TYPE num) #define SMALL 32 /* Must be 32 == 0x20 */ #define SPECIAL 64 /* 0x */
-static char *number(char *buf, unsigned NUM_TYPE num, int base, int size, int precision, int type) +static char *number(char *buf, char *end, unsigned NUM_TYPE num, + int base, int size, int precision, int type) { /* we are called with base 8, 10 or 16, only, thus don't need "G..." */ static const char digits[16] = "0123456789ABCDEF"; /* "GHIJKLMNOPQRSTUVWXYZ"; */ @@ -351,37 +353,63 @@ static char *number(char *buf, unsigned NUM_TYPE num, int base, int size, int pr precision = i; /* leading space padding */ size -= precision; - if (!(type & (ZEROPAD+LEFT))) - while(--size >= 0) - *buf++ = ' '; + if (!(type & (ZEROPAD+LEFT))) { + while (--size >= 0) { + if (buf < end) + *buf = ' '; + ++buf; + } + } /* sign */ - if (sign) - *buf++ = sign; + if (sign) { + if (buf < end) + *buf = sign; + ++buf; + } /* "0x" / "0" prefix */ if (need_pfx) { - *buf++ = '0'; - if (base == 16) - *buf++ = ('X' | locase); + if (buf < end) + *buf = '0'; + ++buf; + if (base == 16) { + if (buf < end) + *buf = ('X' | locase); + ++buf; + } } /* zero or space padding */ if (!(type & LEFT)) { char c = (type & ZEROPAD) ? '0' : ' '; - while (--size >= 0) - *buf++ = c; + while (--size >= 0) { + if (buf < end) + *buf = c; + ++buf; + } } /* hmm even more zero padding? */ - while (i <= --precision) - *buf++ = '0'; + while (i <= --precision) { + if (buf < end) + *buf = '0'; + ++buf; + } /* actual digits of result */ - while (--i >= 0) - *buf++ = tmp[i]; + while (--i >= 0) { + if (buf < end) + *buf = tmp[i]; + ++buf; + + } /* trailing space padding */ - while (--size >= 0) - *buf++ = ' '; + while (--size >= 0) { + if (buf < end) + *buf = ' '; + ++buf; + } return buf; }
-static char *string(char *buf, char *s, int field_width, int precision, int flags) +static char *string(char *buf, char *end, char *s, int field_width, + int precision, int flags) { int len, i;
@@ -391,17 +419,26 @@ static char *string(char *buf, char *s, int field_width, int precision, int flag len = strnlen(s, precision);
if (!(flags & LEFT)) - while (len < field_width--) - *buf++ = ' '; - for (i = 0; i < len; ++i) - *buf++ = *s++; - while (len < field_width--) - *buf++ = ' '; + while (len < field_width--) { + if (buf < end) + *buf = ' '; + ++buf; + } + for (i = 0; i < len; ++i) { + if (buf < end) + *buf = *s++; + ++buf; + } + while (len < field_width--) { + if (buf < end) + *buf = ' '; + ++buf; + } return buf; }
#ifdef CONFIG_CMD_NET -static char *mac_address_string(char *buf, u8 *addr, int field_width, +static char *mac_address_string(char *buf, char *end, u8 *addr, int field_width, int precision, int flags) { char mac_addr[6 * 3]; /* (6 * 2 hex digits), 5 colons and trailing zero */ @@ -415,10 +452,11 @@ static char *mac_address_string(char *buf, u8 *addr, int field_width, } *p = '\0';
- return string(buf, mac_addr, field_width, precision, flags & ~SPECIAL); + return string(buf, end, mac_addr, field_width, precision, + flags & ~SPECIAL); }
-static char *ip6_addr_string(char *buf, u8 *addr, int field_width, +static char *ip6_addr_string(char *buf, char *end, u8 *addr, int field_width, int precision, int flags) { char ip6_addr[8 * 5]; /* (8 * 4 hex digits), 7 colons and trailing zero */ @@ -433,10 +471,11 @@ static char *ip6_addr_string(char *buf, u8 *addr, int field_width, } *p = '\0';
- return string(buf, ip6_addr, field_width, precision, flags & ~SPECIAL); + return string(buf, end, ip6_addr, field_width, precision, + flags & ~SPECIAL); }
-static char *ip4_addr_string(char *buf, u8 *addr, int field_width, +static char *ip4_addr_string(char *buf, char *end, u8 *addr, int field_width, int precision, int flags) { char ip4_addr[4 * 4]; /* (4 * 3 decimal digits), 3 dots and trailing zero */ @@ -454,7 +493,8 @@ static char *ip4_addr_string(char *buf, u8 *addr, int field_width, } *p = '\0';
- return string(buf, ip4_addr, field_width, precision, flags & ~SPECIAL); + return string(buf, end, ip4_addr, field_width, precision, + flags & ~SPECIAL); } #endif
@@ -476,10 +516,12 @@ static char *ip4_addr_string(char *buf, u8 *addr, int field_width, * function pointers are really function descriptors, which contain a * pointer to the real address. */ -static char *pointer(const char *fmt, char *buf, void *ptr, int field_width, int precision, int flags) +static char *pointer(const char *fmt, char *buf, char *end, void *ptr, + int field_width, int precision, int flags) { if (!ptr) - return string(buf, "(null)", field_width, precision, flags); + return string(buf, end, "(null)", field_width, precision, + flags);
#ifdef CONFIG_CMD_NET switch (*fmt) { @@ -487,15 +529,18 @@ static char *pointer(const char *fmt, char *buf, void *ptr, int field_width, int flags |= SPECIAL; /* Fallthrough */ case 'M': - return mac_address_string(buf, ptr, field_width, precision, flags); + return mac_address_string(buf, end, ptr, field_width, + precision, flags); case 'i': flags |= SPECIAL; /* Fallthrough */ case 'I': if (fmt[1] == '6') - return ip6_addr_string(buf, ptr, field_width, precision, flags); + return ip6_addr_string(buf, end, ptr, field_width, + precision, flags); if (fmt[1] == '4') - return ip4_addr_string(buf, ptr, field_width, precision, flags); + return ip4_addr_string(buf, end, ptr, field_width, + precision, flags); flags &= ~SPECIAL; break; } @@ -505,31 +550,35 @@ static char *pointer(const char *fmt, char *buf, void *ptr, int field_width, int field_width = 2*sizeof(void *); flags |= ZEROPAD; } - return number(buf, (unsigned long) ptr, 16, field_width, precision, flags); + return number(buf, end, (unsigned long)ptr, 16, field_width, + precision, flags); }
/** - * vsprintf - Format a string and place it in a buffer - * @buf: The buffer to place the result into - * @fmt: The format string to use - * @args: Arguments for the format string + * Format a string and place it in a buffer (base function) * - * This function follows C99 vsprintf, but has some extensions: + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param args Arguments for the format string + * + * This function follows C99 vsnprintf, but has some extensions: * %pS output the name of a text symbol * %pF output the name of a function pointer * %pR output the address range in a struct resource * - * The function returns the number of characters written - * into @buf. + * The function returns the number of characters which would be + * generated for the given input, excluding the trailing '\0', + * as per ISO C99. * * Call this function if you are already dealing with a va_list. - * You probably want sprintf() instead. + * You probably want snprintf() instead. */ -int vsprintf(char *buf, const char *fmt, va_list args) +int vsnprintf(char *buf, size_t size, const char *fmt, va_list args) { unsigned NUM_TYPE num; int base; - char *str; + char *str, *end;
int flags; /* flags to number() */
@@ -542,10 +591,19 @@ int vsprintf(char *buf, const char *fmt, va_list args) /* 't' added for ptrdiff_t */
str = buf; + end = str + size; + + /* Make sure end is always >= buf */ + if (end < buf) { + end = ((void *)-1); + size = end - buf; + }
for (; *fmt ; ++fmt) { if (*fmt != '%') { - *str++ = *fmt; + if (str < end) + *str = *fmt; + ++str; continue; }
@@ -606,21 +664,32 @@ int vsprintf(char *buf, const char *fmt, va_list args) base = 10;
switch (*fmt) { - case 'c': + case 'c': { + char c; if (!(flags & LEFT)) - while (--field_width > 0) - *str++ = ' '; - *str++ = (unsigned char) va_arg(args, int); - while (--field_width > 0) - *str++ = ' '; + while (--field_width > 0) { + if (str < end) + *str = ' '; + ++str; + } + c = (unsigned char) va_arg(args, int); + if (str < end) + *str = c; + ++str; + while (--field_width > 0) { + if (str < end) + *str = ' '; + ++str; + } continue; - + } case 's': - str = string(str, va_arg(args, char *), field_width, precision, flags); + str = string(str, end, va_arg(args, char *), + field_width, precision, flags); continue;
case 'p': - str = pointer(fmt+1, str, + str = pointer(fmt+1, str, end, va_arg(args, void *), field_width, precision, flags); /* Skip all alphanumeric pointer suffixes */ @@ -639,7 +708,9 @@ int vsprintf(char *buf, const char *fmt, va_list args) continue;
case '%': - *str++ = '%'; + if (str < end) + *str = '%'; + ++str; continue;
/* integer number formats - set up the flags and "break" */ @@ -660,10 +731,14 @@ int vsprintf(char *buf, const char *fmt, va_list args) break;
default: - *str++ = '%'; - if (*fmt) - *str++ = *fmt; - else + if (str < end) + *str = '%'; + ++str; + if (*fmt) { + if (str < end) + *str = *fmt; + ++str; + } else --fmt; continue; } @@ -686,17 +761,124 @@ int vsprintf(char *buf, const char *fmt, va_list args) if (flags & SIGN) num = (signed int) num; } - str = number(str, num, base, field_width, precision, flags); + str = number(str, end, num, base, field_width, precision, + flags); + } + + if (size > 0) { + if (str < end) + *str = '\0'; + else + end[-1] = '\0'; } - *str = '\0'; + /* the trailing null byte doesn't count towards the total */ return str-buf; }
/** - * sprintf - Format a string and place it in a buffer - * @buf: The buffer to place the result into - * @fmt: The format string to use - * @...: Arguments for the format string + * Format a string and place it in a buffer (va_list version) + * + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param args Arguments for the format string + * @return the number of characters which have been written into + * the @buf not including the trailing '\0'. If @size is == 0 the function + * returns 0. + * + * If you're not already dealing with a va_list consider using scnprintf(). + * + * See the vsprintf() documentation for format string extensions over C99. + */ +int vscnprintf(char *buf, size_t size, const char *fmt, va_list args) +{ + int i; + + i = vsnprintf(buf, size, fmt, args); + + if (likely(i < size)) + return i; + if (size != 0) + return size - 1; + return 0; +} + +/** + * Format a string and place it in a buffer + * + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param ... Arguments for the format string + * @return the number of characters which would be + * generated for the given input, excluding the trailing null, + * as per ISO C99. If the return is greater than or equal to + * @size, the resulting string is truncated. + * + * See the vsprintf() documentation for format string extensions over C99. + */ +int snprintf(char *buf, size_t size, const char *fmt, ...) +{ + va_list args; + int i; + + va_start(args, fmt); + i = vsnprintf(buf, size, fmt, args); + va_end(args); + + return i; +} + +/** + * Format a string and place it in a buffer + * + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param ... Arguments for the format string + * + * The return value is the number of characters written into @buf not including + * the trailing '\0'. If @size is == 0 the function returns 0. + * + * See the vsprintf() documentation for format string extensions over C99. + */ + +int scnprintf(char *buf, size_t size, const char *fmt, ...) +{ + va_list args; + int i; + + va_start(args, fmt); + i = vscnprintf(buf, size, fmt, args); + va_end(args); + + return i; +} + +/** + * Format a string and place it in a buffer (va_list version) + * + * @param buf The buffer to place the result into + * @param fmt The format string to use + * @param args Arguments for the format string + * + * The function returns the number of characters written + * into @buf. Use vsnprintf() or vscnprintf() in order to avoid + * buffer overflows. + * + * If you're not already dealing with a va_list consider using sprintf(). + */ +int vsprintf(char *buf, const char *fmt, va_list args) +{ + return vsnprintf(buf, INT_MAX, fmt, args); +} + +/** + * Format a string and place it in a buffer + * + * @param buf The buffer to place the result into + * @param fmt The format string to use + * @param ... Arguments for the format string * * The function returns the number of characters written * into @buf.
Hi Simon,
On 24/09/11 03:38, Simon Glass wrote:
From: Sonny Rao sonnyrao@chromium.org
From: Sonny Rao sonnyrao@chromium.org
These functions are useful in U-Boot because they allow a graceful failure rather than an unpredictable stack overflow when printf() buffers are exceeded.
Mostly copied from the Linux kernel. I copied vscnprintf and scnprintf so we can change printf and vprintf to use the safe implementation but still return the correct values.
Have you checked for license compatibility? U-Boot is GPLv2+ and (most) of Linux is GPLv2 - You may not be legally permitted to do this
Regards,
Graeme
On Fri, Sep 23, 2011 at 4:56 PM, Graeme Russ graeme.russ@gmail.com wrote:
Hi Simon,
On 24/09/11 03:38, Simon Glass wrote:
From: Sonny Rao sonnyrao@chromium.org
From: Sonny Rao sonnyrao@chromium.org
These functions are useful in U-Boot because they allow a graceful failure rather than an unpredictable stack overflow when printf() buffers are exceeded.
Mostly copied from the Linux kernel. I copied vscnprintf and scnprintf so we can change printf and vprintf to use the safe implementation but still return the correct values.
Have you checked for license compatibility? U-Boot is GPLv2+ and (most) of Linux is GPLv2 - You may not be legally permitted to do this
According to the FSF site, GPLv2 is compatible with GPLv3, see: http://www.gnu.org/licenses/quick-guide-gplv3.html
So it's fine to distribute them together.
In reality though, this code in U-boot was already copied from the same file in an older version of the kernel. The license (GPLv2 only) hasn't changed on that file, so U-boot is already distributing what is GPLv2 only code alongside GPLv2+ code -- which as I mentioned above is fine.
The code here is derived from a later version of that same file, so I don't believe integrating this patch into U-boot actually changes anything with respect to licensing of this code.
Sonny
Hi Sonny,
On Thu, Sep 29, 2011 at 9:26 AM, Sonny Rao sonnyrao@chromium.org wrote:
On Fri, Sep 23, 2011 at 4:56 PM, Graeme Russ graeme.russ@gmail.com wrote:
Hi Simon,
On 24/09/11 03:38, Simon Glass wrote:
From: Sonny Rao sonnyrao@chromium.org
From: Sonny Rao sonnyrao@chromium.org
These functions are useful in U-Boot because they allow a graceful failure rather than an unpredictable stack overflow when printf() buffers are exceeded.
Mostly copied from the Linux kernel. I copied vscnprintf and scnprintf so we can change printf and vprintf to use the safe implementation but still return the correct values.
Have you checked for license compatibility? U-Boot is GPLv2+ and (most) of Linux is GPLv2 - You may not be legally permitted to do this
According to the FSF site, GPLv2 is compatible with GPLv3, see: http://www.gnu.org/licenses/quick-guide-gplv3.html
So it's fine to distribute them together.
Not so fast:
"GPLv2 is compatible with GPLv3 if the program allows you to choose "any later version" of the GPL, which is the case for most software released under this license"
'most', not 'all'
The key phrase is "any later version" - If this is not in the GPLv2 license text for the code your copying, you cannot license the derivative work under GPLv3 (or GPLv2+)
In reality though, this code in U-boot was already copied from the same file in an older version of the kernel. The license (GPLv2 only) hasn't changed on that file, so U-boot is already distributing what is GPLv2 only code alongside GPLv2+ code -- which as I mentioned above is fine.
As long as you are not incorporating GPLv2 code into a file licensed under GPLv2+ and distributing the result as GPLv2+ or GPLv3.
The code here is derived from a later version of that same file, so I don't believe integrating this patch into U-boot actually changes anything with respect to licensing of this code.
I think U-Boot will have great difficulty going past GPLv2 due to the large volume of Linus code already in U-Boot - We would need to either get the original code relicensed GPLv2+ or rewrite it
And from the COPYING file in the Linux source:
Also note that the only valid version of the GPL as far as the kernel is concerned is _this_ particular version of the license (ie v2, not v2.2 or v3.x or whatever), unless explicitly otherwise stated.
and linux/lib/vsprintf.c:
/* * linux/lib/vsprintf.c * * Copyright (C) 1991, 1992 Linus Torvalds */
/* vsprintf.c -- Lars Wirzenius & Linus Torvalds. */ /* * Wirzenius wrote this portably, Torvalds fucked it up :-) */
/* * Fri Jul 13 2001 Crutcher Dunnavant crutcher+kernel@datastacks.com * - changed to provide snprintf and vsnprintf functions * So Feb 1 16:51:32 CET 2004 Juergen Quade quade@hsnr.de * - scnprintf and vscnprintf */
No 'any later version'
Now U-Boot COPYING has:
U-Boot is Free Software. It is copyrighted by Wolfgang Denk and many others who contributed code (see the actual source code for details). You can redistribute U-Boot and/or modify it under the terms of version 2 of the GNU General Public License as published by the Free Software Foundation. Most of it can also be distributed, ----------------------------------- at your option, under any later version of the GNU General Public ----------------------------------------------------------------- License -- see individual files for exceptions. -----------------------------------------------
So U-Boot is also GPLv2, but parts (like Linux) are GPLv2+
My point was there is a long term 'vision' for U-Boot to go GPLv3, and part of that vision is to reject any future GPLv2 only licensed code submitted. If we want the vision to be realised, I think we need to look at how we build new GPLv2+ code in new files rather than tying new code into GPLv2 only files which may, potentially, lock that code down. Although as the writer and copyright holder of a modification you may be free to move it over at a later date anyway - I don't know, and I think that's in the 'ask a lawyer' territory.
So yes, you are right - In terms of license compliance, we are OK as U-Boot is currently GPLv2 - In terms of U-Boot going to GPLv3, we are digging a bigger hole :)
Regards,
Graeme
On Wed, Sep 28, 2011 at 5:00 PM, Graeme Russ graeme.russ@gmail.com wrote:
Hi Sonny,
On Thu, Sep 29, 2011 at 9:26 AM, Sonny Rao sonnyrao@chromium.org wrote:
On Fri, Sep 23, 2011 at 4:56 PM, Graeme Russ graeme.russ@gmail.com wrote:
Hi Simon,
On 24/09/11 03:38, Simon Glass wrote:
From: Sonny Rao sonnyrao@chromium.org
From: Sonny Rao sonnyrao@chromium.org
These functions are useful in U-Boot because they allow a graceful failure rather than an unpredictable stack overflow when printf() buffers are exceeded.
Mostly copied from the Linux kernel. I copied vscnprintf and scnprintf so we can change printf and vprintf to use the safe implementation but still return the correct values.
Have you checked for license compatibility? U-Boot is GPLv2+ and (most) of Linux is GPLv2 - You may not be legally permitted to do this
According to the FSF site, GPLv2 is compatible with GPLv3, see: http://www.gnu.org/licenses/quick-guide-gplv3.html
So it's fine to distribute them together.
Not so fast:
"GPLv2 is compatible with GPLv3 if the program allows you to choose "any later version" of the GPL, which is the case for most software released under this license"
'most', not 'all'
The key phrase is "any later version" - If this is not in the GPLv2 license text for the code your copying, you cannot license the derivative work under GPLv3 (or GPLv2+)
I'm a bit confused here... I was talking about distribution, not re-licensing. I'm certainly not saying U-boot should be trying to re-license GPLv2 only code. But the fact is that U-boot is already contains GPLv2 and GPLv2+ code and it is being distributed together.
In reality though, this code in U-boot was already copied from the same file in an older version of the kernel. The license (GPLv2 only) hasn't changed on that file, so U-boot is already distributing what is GPLv2 only code alongside GPLv2+ code -- which as I mentioned above is fine.
As long as you are not incorporating GPLv2 code into a file licensed under GPLv2+ and distributing the result as GPLv2+ or GPLv3.
My argument is that this code in U-boot was already GPLv2 only since it came from a GPLv2 only file in the kernel. I don't think that we're applying GPLv2 code into a file licensed under GPLv2+. Perhaps we could add an explicit comment stating that the code in lib/vsprintf.c is GPLv2 only so nobody gets the wrong idea.
The code here is derived from a later version of that same file, so I don't believe integrating this patch into U-boot actually changes anything with respect to licensing of this code.
I think U-Boot will have great difficulty going past GPLv2 due to the large volume of Linus code already in U-Boot - We would need to either get the original code relicensed GPLv2+ or rewrite it
And from the COPYING file in the Linux source:
Also note that the only valid version of the GPL as far as the kernel is concerned is _this_ particular version of the license (ie v2, not v2.2 or v3.x or whatever), unless explicitly otherwise stated.
and linux/lib/vsprintf.c:
/* * linux/lib/vsprintf.c * * Copyright (C) 1991, 1992 Linus Torvalds */
/* vsprintf.c -- Lars Wirzenius & Linus Torvalds. */ /* * Wirzenius wrote this portably, Torvalds fucked it up :-) */
/* * Fri Jul 13 2001 Crutcher Dunnavant crutcher+kernel@datastacks.com * - changed to provide snprintf and vsnprintf functions * So Feb 1 16:51:32 CET 2004 Juergen Quade quade@hsnr.de * - scnprintf and vscnprintf */
No 'any later version'
Yes, as I said before, this code is definitely GPLv2 only. I don't think we're disagreeing here.
Now U-Boot COPYING has:
U-Boot is Free Software. It is copyrighted by Wolfgang Denk and many others who contributed code (see the actual source code for details). You can redistribute U-Boot and/or modify it under the terms of version 2 of the GNU General Public License as published by the Free Software Foundation. Most of it can also be distributed, ----------------------------------- at your option, under any later version of the GNU General Public
License -- see individual files for exceptions.
So U-Boot is also GPLv2, but parts (like Linux) are GPLv2+
My point was there is a long term 'vision' for U-Boot to go GPLv3, and part of that vision is to reject any future GPLv2 only licensed code submitted. If we want the vision to be realised, I think we need to look at how we build new GPLv2+ code in new files rather than tying new code into GPLv2 only files which may, potentially, lock that code down. Although as the writer and copyright holder of a modification you may be free to move it over at a later date anyway - I don't know, and I think that's in the 'ask a lawyer' territory.
So yes, you are right - In terms of license compliance, we are OK as U-Boot is currently GPLv2 - In terms of U-Boot going to GPLv3, we are digging a bigger hole :)
Ok, then that's up to the U-boot community to decide if they wish to remove all GPLv2 only code, but I think that's a separate issue from what you originally brought up. I think we're in agreement that there aren't any licensing considerations with respect to maintaining the existing license which is GPLv2 only for lib/vsprintf.c.
Sonny
Hi Sonny,
On Thu, Sep 29, 2011 at 10:38 AM, Sonny Rao sonnyrao@chromium.org wrote:
On Wed, Sep 28, 2011 at 5:00 PM, Graeme Russ graeme.russ@gmail.com wrote:
Hi Sonny,
[snip]
So yes, you are right - In terms of license compliance, we are OK as U-Boot is currently GPLv2 - In terms of U-Boot going to GPLv3, we are digging a bigger hole :)
Ok, then that's up to the U-boot community to decide if they wish to remove all GPLv2 only code, but I think that's a separate issue from what you originally brought up. I think we're in agreement that there aren't any licensing considerations with respect to maintaining the existing license which is GPLv2 only for lib/vsprintf.c.
Agree 100%
Regards,
Graeme
From: Sonny Rao sonnyrao@chromium.org
From: Sonny Rao sonnyrao@chromium.org
utilize the added vscnprintf functions to avoid buffer overruns The implementation is fairly dumb in that it doesn't detect that the buffer is too small, but at least will not cause crashes.
Signed-off-by: Simon Glass sjg@chromium.org --- common/console.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/common/console.c b/common/console.c index 8c650e0..6057e9a 100644 --- a/common/console.c +++ b/common/console.c @@ -212,7 +212,7 @@ int serial_printf(const char *fmt, ...) /* For this to work, printbuffer must be larger than * anything we ever want to print. */ - i = vsprintf(printbuffer, fmt, args); + i = vscnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args); va_end(args);
serial_puts(printbuffer); @@ -281,7 +281,7 @@ int fprintf(int file, const char *fmt, ...) /* For this to work, printbuffer must be larger than * anything we ever want to print. */ - i = vsprintf(printbuffer, fmt, args); + i = vscnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args); va_end(args);
/* Send to desired file */ @@ -376,7 +376,7 @@ int printf(const char *fmt, ...) /* For this to work, printbuffer must be larger than * anything we ever want to print. */ - i = vsprintf(printbuffer, fmt, args); + i = vscnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args); va_end(args);
/* Print the string */ @@ -392,7 +392,7 @@ int vprintf(const char *fmt, va_list args) /* For this to work, printbuffer must be larger than * anything we ever want to print. */ - i = vsprintf(printbuffer, fmt, args); + i = vscnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args);
/* Print the string */ puts(printbuffer); @@ -459,7 +459,7 @@ inline void dbg(const char *fmt, ...) /* For this to work, printbuffer must be larger than * anything we ever want to print. */ - i = vsprintf(printbuffer, fmt, args); + i = vsnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args); va_end(args);
if ((screen + sizeof(screen) - 1 - cursor)
On Sep 23, 2011, at 12:38 PM, Simon Glass wrote:
From: Sonny Rao sonnyrao@chromium.org
From: Sonny Rao sonnyrao@chromium.org
utilize the added vscnprintf functions to avoid buffer overruns The implementation is fairly dumb in that it doesn't detect that the buffer is too small, but at least will not cause crashes.
Signed-off-by: Simon Glass sjg@chromium.org
common/console.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
If these are from Sonny, they really should have a Signed-off-by from him.
- k
Hi Kumar,
On Fri, Sep 23, 2011 at 11:36 AM, Kumar Gala galak@kernel.crashing.org wrote:
On Sep 23, 2011, at 12:38 PM, Simon Glass wrote:
From: Sonny Rao sonnyrao@chromium.org
From: Sonny Rao sonnyrao@chromium.org
utilize the added vscnprintf functions to avoid buffer overruns The implementation is fairly dumb in that it doesn't detect that the buffer is too small, but at least will not cause crashes.
Signed-off-by: Simon Glass sjg@chromium.org
common/console.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
If these are from Sonny, they really should have a Signed-off-by from him.
OK will fix that with next version thanks. The commit in my tree says Sonny but perhaps git format-patch --signoff is not doing what I expect.
Regards, Simon
- k
On Friday, September 23, 2011 13:38:51 Simon Glass wrote:
--- a/common/console.c +++ b/common/console.c @@ -212,7 +212,7 @@ int serial_printf(const char *fmt, ...) /* For this to work, printbuffer must be larger than * anything we ever want to print. */
- i = vsprintf(printbuffer, fmt, args);
- i = vscnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args);
i think sizeof(printbuffer) would be better. same goes for all the other changes here. -mike
Hi Mike,
On Fri, Sep 23, 2011 at 1:31 PM, Mike Frysinger vapier@gentoo.org wrote:
On Friday, September 23, 2011 13:38:51 Simon Glass wrote:
--- a/common/console.c +++ b/common/console.c @@ -212,7 +212,7 @@ int serial_printf(const char *fmt, ...) /* For this to work, printbuffer must be larger than * anything we ever want to print. */
- i = vsprintf(printbuffer, fmt, args);
- i = vscnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args);
i think sizeof(printbuffer) would be better. same goes for all the other changes here. -mike
Yes, indeed. Could we go as far as removing CONFIG_SYS_PBSIZE, and just use a standard value?
Regards, Simon
On Friday, September 23, 2011 16:41:50 Simon Glass wrote:
On Fri, Sep 23, 2011 at 1:31 PM, Mike Frysinger wrote:
On Friday, September 23, 2011 13:38:51 Simon Glass wrote:
--- a/common/console.c +++ b/common/console.c @@ -212,7 +212,7 @@ int serial_printf(const char *fmt, ...) /* For this to work, printbuffer must be larger than * anything we ever want to print. */
i = vsprintf(printbuffer, fmt, args);
i = vscnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args);i think sizeof(printbuffer) would be better. same goes for all the other changes here. -mike
Yes, indeed. Could we go as far as removing CONFIG_SYS_PBSIZE, and just use a standard value?
in the context of I/O funcs, CONFIG_SYS_PBSIZE is the current standard -mike
Hi Mike,
On Fri, Sep 23, 2011 at 3:36 PM, Mike Frysinger vapier@gentoo.org wrote:
On Friday, September 23, 2011 16:41:50 Simon Glass wrote:
On Fri, Sep 23, 2011 at 1:31 PM, Mike Frysinger wrote:
On Friday, September 23, 2011 13:38:51 Simon Glass wrote:
--- a/common/console.c +++ b/common/console.c @@ -212,7 +212,7 @@ int serial_printf(const char *fmt, ...) /* For this to work, printbuffer must be larger than * anything we ever want to print. */
- i = vsprintf(printbuffer, fmt, args);
- i = vscnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args);
i think sizeof(printbuffer) would be better. same goes for all the other changes here. -mike
Yes, indeed. Could we go as far as removing CONFIG_SYS_PBSIZE, and just use a standard value?
in the context of I/O funcs, CONFIG_SYS_PBSIZE is the current standard -mike
OK - is there a reason why boards can redefine this? Many many boards do. It seems like something that should just be given a sensible value.
Regards, Simon
Dear Simon Glass,
In message CAPnjgZ2D8MYS7wHMFUnzVhXK_tcmv7GayJyEormkPDNA78p7JQ@mail.gmail.com you wrote:
in the context of I/O funcs, CONFIG_SYS_PBSIZE is the current standard
OK - is there a reason why boards can redefine this? Many many boards do. It seems like something that should just be given a sensible value.
Indeed - that's exactly what the board specific definitions are supposed to do.
Best regards,
Wolfgang Denk
Dear Simon Glass,
In message CAPnjgZ0-8wU4Hj3pdmfNMWnj4EPUmQ69U_UARaRt5CbqQv0Ofg@mail.gmail.com you wrote:
Yes, indeed. Could we go as far as removing CONFIG_SYS_PBSIZE, and just use a standard value?
If you can find one that fits for all boards? Keep in mind that printf() gets used before relocation, when available stack space may be _very_ limited.
Best regards,
Wolfgang Denk
HI Wolfgang,
On Sun, Sep 25, 2011 at 1:14 PM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message CAPnjgZ0-8wU4Hj3pdmfNMWnj4EPUmQ69U_UARaRt5CbqQv0Ofg@mail.gmail.com you wrote:
Yes, indeed. Could we go as far as removing CONFIG_SYS_PBSIZE, and just use a standard value?
If you can find one that fits for all boards? Keep in mind that printf() gets used before relocation, when available stack space may be _very_ limited.
Yes that is a problem. Perhaps we could changes things so that this CONFIG really only matters prior to relocation (see other thread with Albert) and we could just choose a suitable small value like 256, which seems to be the minimum for most boards.
Regards, Simon
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "And it should be the law: If you use the word `paradigm' without knowing what the dictionary says it means, you go to jail. No exceptions." - David Jones @ Megatest Corporation
Dear Simon Glass,
In message CAPnjgZ1Q89Ew2Be2QcjoWF3Nyb+sJ-SxfFunZturMwdLxBc_Wg@mail.gmail.com you wrote:
If you can find one that fits for all boards? =A0Keep in mind that printf() gets used before relocation, when available stack space may be _very_ limited.
Yes that is a problem. Perhaps we could changes things so that this CONFIG really only matters prior to relocation (see other thread with Albert) and we could just choose a suitable small value like 256, which seems to be the minimum for most boards.
You are probably addressing a non-issue. How many related bug reports can you find in the last decade or so?
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Mon, Sep 26, 2011 at 11:47 AM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message CAPnjgZ1Q89Ew2Be2QcjoWF3Nyb+sJ-SxfFunZturMwdLxBc_Wg@mail.gmail.com you wrote:
If you can find one that fits for all boards? =A0Keep in mind that printf() gets used before relocation, when available stack space may be _very_ limited.
Yes that is a problem. Perhaps we could changes things so that this CONFIG really only matters prior to relocation (see other thread with Albert) and we could just choose a suitable small value like 256, which seems to be the minimum for most boards.
You are probably addressing a non-issue. How many related bug reports can you find in the last decade or so?
Yes probably, it isn't important and is off-topic from my patch anyway.
Regards, Simon
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de All your people must learn before you can reach for the stars. -- Kirk, "The Gamesters of Triskelion", stardate 3259.2
This tidies up network code to use snprintf() in most cases instead of sprintf(). A few functions remain as they require header file changes.
Signed-off-by: Simon Glass sjg@chromium.org --- net/eth.c | 10 +++++++--- net/net.c | 15 ++++++++++----- net/nfs.c | 3 ++- net/tftp.c | 3 ++- 4 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/net/eth.c b/net/eth.c index 5911b1c..30711d4 100644 --- a/net/eth.c +++ b/net/eth.c @@ -49,7 +49,7 @@ int eth_setenv_enetaddr(char *name, const uchar *enetaddr) { char buf[20];
- sprintf(buf, "%pM", enetaddr); + snprintf(buf, sizeof(buf), "%pM", enetaddr);
return setenv(name, buf); } @@ -58,7 +58,9 @@ int eth_getenv_enetaddr_by_index(const char *base_name, int index, uchar *enetaddr) { char enetvar[32]; - sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); + + snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr", + base_name, index); return eth_getenv_enetaddr(enetvar, enetaddr); }
@@ -68,7 +70,9 @@ static int eth_mac_skip(int index) { char enetvar[15]; char *skip_state; - sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index); + + snprintf(enetvar, sizeof(enetvar), + index ? "eth%dmacskip" : "ethmacskip", index); return ((skip_state = getenv(enetvar)) != NULL); }
diff --git a/net/net.c b/net/net.c index 7a60583..28e45c0 100644 --- a/net/net.c +++ b/net/net.c @@ -573,10 +573,12 @@ restart: printf("Bytes transferred = %ld (%lx hex)\n", NetBootFileXferSize, NetBootFileXferSize); - sprintf(buf, "%lX", NetBootFileXferSize); + snprintf(buf, sizeof(buf), "%lX", + NetBootFileXferSize); setenv("filesize", buf);
- sprintf(buf, "%lX", (unsigned long)load_addr); + snprintf(buf, sizeof(buf), "%lX", + (unsigned long)load_addr); setenv("fileaddr", buf); } eth_halt(); @@ -950,7 +952,8 @@ int CDPSendTrigger(void) #ifdef CONFIG_CDP_DEVICE_ID *s++ = htons(CDP_DEVICE_ID_TLV); *s++ = htons(CONFIG_CDP_DEVICE_ID); - sprintf(buf, CONFIG_CDP_DEVICE_ID_PREFIX "%pm", NetOurEther); + snprintf(buf, sizeof(buf), CONFIG_CDP_DEVICE_ID_PREFIX "%pm", + NetOurEther); memcpy((uchar *)s, buf, 16); s += 16 / 2; #endif @@ -958,7 +961,7 @@ int CDPSendTrigger(void) #ifdef CONFIG_CDP_PORT_ID *s++ = htons(CDP_PORT_ID_TLV); memset(buf, 0, sizeof(buf)); - sprintf(buf, CONFIG_CDP_PORT_ID, eth_get_dev_index()); + snprintf(buf, sizeof(buf), CONFIG_CDP_PORT_ID, eth_get_dev_index()); len = strlen(buf); if (len & 1) /* make it even */ len++; @@ -1516,7 +1519,9 @@ NetReceive(volatile uchar *inpkt, int len) #ifdef CONFIG_KEEP_SERVERADDR if (NetServerIP == NetArpWaitPacketIP) { char buf[20]; - sprintf(buf, "%pM", arp->ar_data); + + snprintf(buf, sizeof(buf), "%pM", + arp->ar_data); setenv("serveraddr", buf); } #endif diff --git a/net/nfs.c b/net/nfs.c index f76f60d..d1e894e 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -688,7 +688,8 @@ NfsStart (void) }
if (BootFile[0] == '\0') { - sprintf (default_filename, "/nfsroot/%02lX%02lX%02lX%02lX.img", + snprintf(default_filename, sizeof(default_filename), + "/nfsroot/%02lX%02lX%02lX%02lX.img", NetOurIP & 0xFF, (NetOurIP >> 8) & 0xFF, (NetOurIP >> 16) & 0xFF, diff --git a/net/tftp.c b/net/tftp.c index a893e02..c4f6a59 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -591,7 +591,8 @@ TftpStart(void)
TftpRemoteIP = NetServerIP; if (BootFile[0] == '\0') { - sprintf(default_filename, "%02lX%02lX%02lX%02lX.img", + snprintf(default_filename, sizeof(default_filename), + "%02lX%02lX%02lX%02lX.img", NetOurIP & 0xFF, (NetOurIP >> 8) & 0xFF, (NetOurIP >> 16) & 0xFF,
On Friday, September 23, 2011 13:38:52 Simon Glass wrote:
This tidies up network code to use snprintf() in most cases instead of sprintf(). A few functions remain as they require header file changes.
NAK to most of these. we pick local sized buffers that are known to not overflow, or require circumstances that aren't really feasible.
3 examples (which are the first 3 changes in this patch) below ...
--- a/net/eth.c +++ b/net/eth.c
char buf[20];
- sprintf(buf, "%pM", enetaddr);
- snprintf(buf, sizeof(buf), "%pM", enetaddr);
a mac address will not take more than 19 bytes. unless the sprintf code is completely busted, but if that's the case, we should fix that instead since it'd be pretty fundamentally screwed.
char enetvar[32];
- sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
- snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr",
base_name, index);
in order for this to overflow, we have to have 1000+ eth devices (maybe more? i'd have to read the code closer)
char enetvar[15];
- sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index);
- snprintf(enetvar, sizeof(enetvar),
index ? "eth%dmacskip" : "ethmacskip", index);
in order for this to overflow, we have to have 10000+ eth devices
please look at the realistic needs rather than blanket converting to snprintf -mike
Hi Mike,
On Fri, Sep 23, 2011 at 11:15 AM, Mike Frysinger vapier@gentoo.org wrote:
On Friday, September 23, 2011 13:38:52 Simon Glass wrote:
This tidies up network code to use snprintf() in most cases instead of sprintf(). A few functions remain as they require header file changes.
NAK to most of these. we pick local sized buffers that are known to not overflow, or require circumstances that aren't really feasible.
Yes that's why I sent this patch on the end, to see how far this should be taken.
3 examples (which are the first 3 changes in this patch) below ...
--- a/net/eth.c +++ b/net/eth.c
char buf[20];
- sprintf(buf, "%pM", enetaddr);
- snprintf(buf, sizeof(buf), "%pM", enetaddr);
a mac address will not take more than 19 bytes. unless the sprintf code is completely busted, but if that's the case, we should fix that instead since it'd be pretty fundamentally screwed.
OK (18 bytes?)
char enetvar[32];
- sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
- snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr",
- base_name, index);
in order for this to overflow, we have to have 1000+ eth devices (maybe more? i'd have to read the code closer)
Or base_name could be longer.
char enetvar[15];
- sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index);
- snprintf(enetvar, sizeof(enetvar),
- index ? "eth%dmacskip" : "ethmacskip", index);
in order for this to overflow, we have to have 10000+ eth devices
please look at the realistic needs rather than blanket converting to snprintf
OK, this is fair enough. There are a couple of functions like ip_to_string() which don't get passed a length. I would prefer to see this done, so we can use snprintf() or assert() since the code then becomes more robust. But there are no bugs there at present. Thoughts?
Regards, Simon
-mike
On Friday, September 23, 2011 14:30:09 Simon Glass wrote:
On Fri, Sep 23, 2011 at 11:15 AM, Mike Frysinger wrote:
On Friday, September 23, 2011 13:38:52 Simon Glass wrote:
--- a/net/eth.c +++ b/net/eth.c
char buf[20];
sprintf(buf, "%pM", enetaddr);
snprintf(buf, sizeof(buf), "%pM", enetaddr);a mac address will not take more than 19 bytes. unless the sprintf code is completely busted, but if that's the case, we should fix that instead since it'd be pretty fundamentally screwed.
OK (18 bytes?)
right ... i meant there's 19 available (since 20 makes NUL), and there's no way it should hit that 19 limit.
char enetvar[32];
sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr",base_name, index);in order for this to overflow, we have to have 1000+ eth devices (maybe more? i'd have to read the code closer)
Or base_name could be longer.
true, but base_name atm is supposed to be "eth" or "usbeth". i would hope we'd be diligent about device naming conventions rather than letting someone slip in "mysuperawesomeboardeth" (although that still wouldn't overflow ;]).
char enetvar[15];
sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index);
snprintf(enetvar, sizeof(enetvar),index ? "eth%dmacskip" : "ethmacskip", index);in order for this to overflow, we have to have 10000+ eth devices
please look at the realistic needs rather than blanket converting to snprintf
OK, this is fair enough. There are a couple of functions like ip_to_string() which don't get passed a length. I would prefer to see this done, so we can use snprintf() or assert() since the code then becomes more robust. But there are no bugs there at present. Thoughts?
for funcs that do generally handle arbitrary data, i don't mind including len specs, but when we're using known standards, i'd prefer to stick to the smaller/simpler code. this is a thin boot loader that is meant to be fast, and there are plenty of ways to bring down the system otherwise (and i don't think these cases are generally a robustness concern).
don't know if wolfgang has an opinion. -mike
Hi Mike,
On Fri, Sep 23, 2011 at 1:09 PM, Mike Frysinger vapier@gentoo.org wrote:
On Friday, September 23, 2011 14:30:09 Simon Glass wrote:
On Fri, Sep 23, 2011 at 11:15 AM, Mike Frysinger wrote:
On Friday, September 23, 2011 13:38:52 Simon Glass wrote:
--- a/net/eth.c +++ b/net/eth.c
char buf[20];
- sprintf(buf, "%pM", enetaddr);
- snprintf(buf, sizeof(buf), "%pM", enetaddr);
a mac address will not take more than 19 bytes. unless the sprintf code is completely busted, but if that's the case, we should fix that instead since it'd be pretty fundamentally screwed.
OK (18 bytes?)
right ... i meant there's 19 available (since 20 makes NUL), and there's no way it should hit that 19 limit.
char enetvar[32];
- sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
- snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr",
- base_name, index);
in order for this to overflow, we have to have 1000+ eth devices (maybe more? i'd have to read the code closer)
Or base_name could be longer.
true, but base_name atm is supposed to be "eth" or "usbeth". i would hope we'd be diligent about device naming conventions rather than letting someone slip in "mysuperawesomeboardeth" (although that still wouldn't overflow ;]).
Well here I feel that it is pushing it a bit to have a function that builds a string based on arguments over which it has no control. I suppose an assert(strlen(base_name) < 23) or whatever would do it. But isn't snprintf() better?
char enetvar[15];
- sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index);
- snprintf(enetvar, sizeof(enetvar),
- index ? "eth%dmacskip" : "ethmacskip", index);
in order for this to overflow, we have to have 10000+ eth devices
please look at the realistic needs rather than blanket converting to snprintf
OK, this is fair enough. There are a couple of functions like ip_to_string() which don't get passed a length. I would prefer to see this done, so we can use snprintf() or assert() since the code then becomes more robust. But there are no bugs there at present. Thoughts?
for funcs that do generally handle arbitrary data, i don't mind including len specs, but when we're using known standards, i'd prefer to stick to the smaller/simpler code. this is a thin boot loader that is meant to be fast, and there are plenty of ways to bring down the system otherwise (and i don't think these cases are generally a robustness concern).
Well that argues towards using assert() in these cases rather than just ignoring them. I'm keen to try to make U-Boot more testable. I do understand it is just a 'thin' boot loader, but it has quite a few features now, and it if breaks, the system won't boot.
I do agree there is a difference between telling devs about a bug / overflow at build/test time and bloating the code in production. Either/or is good with me, but I'm not entirely comfortable with just ignoring these issues and hoping they don't bite us. It's not like we have a test suite to check it.
don't know if wolfgang has an opinion.
<insert here>
Regards, Simon
-mike
Hi Simon,
Le 23/09/2011 19:38, Simon Glass a écrit :
The printf family of functions in U-Boot cannot deal with a situation where the caller provides a buffer which turns out to be too small for the format string. This can result in buffer overflows, stack overflows and other bad behavior.
Indeed overruns can lead to bad behaviors, but in any case, it can never be recovered, because at the root, the problem is that the caller provided inconsistent arguments to printf.
So in essence, you're 'fixing' printf for a design error in printf's caller, instead of fixing the design error.
Amicalement,
Hi Albert,
On Fri, Sep 23, 2011 at 1:40 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hi Simon,
Le 23/09/2011 19:38, Simon Glass a écrit :
The printf family of functions in U-Boot cannot deal with a situation where the caller provides a buffer which turns out to be too small for the format string. This can result in buffer overflows, stack overflows and other bad behavior.
Indeed overruns can lead to bad behaviors, but in any case, it can never be recovered, because at the root, the problem is that the caller provided inconsistent arguments to printf.
Recovery is one thing, but I would settle for just not crashing, which is the purpose of this patch. We could also easily WARN if that were considered appropriate here.
So in essence, you're 'fixing' printf for a design error in printf's caller, instead of fixing the design error.
Well, the nature of a function is that it cannot know what arguments might be passed to it. It can only assert(), limit check, etc. A limit check is what this patch aims to add.
Regards, Simon
Amicalement,
Albert.
Le 23/09/2011 22:46, Simon Glass a écrit :
Hi Albert,
On Fri, Sep 23, 2011 at 1:40 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hi Simon,
Le 23/09/2011 19:38, Simon Glass a écrit :
The printf family of functions in U-Boot cannot deal with a situation where the caller provides a buffer which turns out to be too small for the format string. This can result in buffer overflows, stack overflows and other bad behavior.
Indeed overruns can lead to bad behaviors, but in any case, it can never be recovered, because at the root, the problem is that the caller provided inconsistent arguments to printf.
Recovery is one thing, but I would settle for just not crashing, which is the purpose of this patch. We could also easily WARN if that were considered appropriate here.
So in essence, you're 'fixing' printf for a design error in printf's caller, instead of fixing the design error.
Well, the nature of a function is that it cannot know what arguments might be passed to it. It can only assert(), limit check, etc. A limit check is what this patch aims to add.
I do agree that as a rule, a function should check its arguments, but as any rule, this one has understated assumptions. Here, one assumption is that calls to the functions cannot be made compliant, and therefore it falls to the function to ensure this compliance; and another one is that the function can actually check this conformance.
The first assumption is correct in an OS or general-purpose library where the number of callers is virtually unlimited and there is no way to make sure all calls are conforming.
In U-Boot however, the number of callers is bound and known (unless you're thinking of standalone U-Boot apps, but I don't see this as a use case so frequent that it warrants a 'fix')
The second assumption is false by nature for printf(), which simply cannot check its input buffer (OTOH *nprintf() does, and it is its job).
Besides, there *is* a sub-family of printf functions which are dedicated to the case where buffers provided may be too small for the print output -- meaning that the issue is known and an easy fix exists if cropping the output is allowable.
So basically the choice is between:
- adding code to the printf() family to try and fix an issue that it is fundamentally unable to properly fix, and for which a solution exists, or
- grepping and fixing calls to *sprintf() in U-Boot that do not respect the known contraints of printf(), by resizing the buffer or calling *snprintf() instead.
I am definitely not in favor of the first option concerning U-Boot.
Regards, Simon
Amicalement,
Hi Albert,
On Sat, Sep 24, 2011 at 2:37 AM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Le 23/09/2011 22:46, Simon Glass a écrit :
Hi Albert,
On Fri, Sep 23, 2011 at 1:40 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hi Simon,
Le 23/09/2011 19:38, Simon Glass a écrit :
The printf family of functions in U-Boot cannot deal with a situation where the caller provides a buffer which turns out to be too small for the format string. This can result in buffer overflows, stack overflows and other bad behavior.
Indeed overruns can lead to bad behaviors, but in any case, it can never be recovered, because at the root, the problem is that the caller provided inconsistent arguments to printf.
Recovery is one thing, but I would settle for just not crashing, which is the purpose of this patch. We could also easily WARN if that were considered appropriate here.
So in essence, you're 'fixing' printf for a design error in printf's caller, instead of fixing the design error.
Well, the nature of a function is that it cannot know what arguments might be passed to it. It can only assert(), limit check, etc. A limit check is what this patch aims to add.
I do agree that as a rule, a function should check its arguments, but as any rule, this one has understated assumptions. Here, one assumption is that calls to the functions cannot be made compliant, and therefore it falls to the function to ensure this compliance; and another one is that the function can actually check this conformance.
There is also the issue of programmer error, or unexpected situations, and graceful failure in these cases. We want to avoid hard-to-find memory/stack corruption bugs if the cost of doing so is moderate.
The first assumption is correct in an OS or general-purpose library where the number of callers is virtually unlimited and there is no way to make sure all calls are conforming.
In U-Boot however, the number of callers is bound and known (unless you're thinking of standalone U-Boot apps, but I don't see this as a use case so frequent that it warrants a 'fix')
Yes, and one can inspect each call and decide if the nprint() variants are preferable in each case.
The second assumption is false by nature for printf(), which simply cannot check its input buffer (OTOH *nprintf() does, and it is its job).
Besides, there *is* a sub-family of printf functions which are dedicated to the case where buffers provided may be too small for the print output -- meaning that the issue is known and an easy fix exists if cropping the output is allowable.
Yes, or even if cropping is not allowed, we might expect a nice error, rather than a crash sometime later when the stack corruption or data space overwriting causes problems.
So basically the choice is between:
- adding code to the printf() family to try and fix an issue that it is
fundamentally unable to properly fix, and for which a solution exists, or
- grepping and fixing calls to *sprintf() in U-Boot that do not respect the
known contraints of printf(), by resizing the buffer or calling *snprintf() instead.
I am definitely not in favor of the first option concerning U-Boot.
Sounds fine to me. So I think we need the nprintf() variants in there, but the message is not to use them willy nilly. Going back to my patch series, 3/4 is ok, but 4/4 mostly crosses the line. Do I have that right?
By the way, printf() ends up calling the same code, but without limit checking in place. The alternative is to duplicate all the format string processing code (a limit-checking version and an unchecked version) which would be worse.
Amicalement,
Albert.
Regards, Simon
Le 24/09/2011 16:00, Simon Glass a écrit :
So basically the choice is between:
- adding code to the printf() family to try and fix an issue that it is
fundamentally unable to properly fix, and for which a solution exists, or
- grepping and fixing calls to *sprintf() in U-Boot that do not respect the
known contraints of printf(), by resizing the buffer or calling *snprintf() instead.
I am definitely not in favor of the first option concerning U-Boot.
Sounds fine to me. So I think we need the nprintf() variants in there, but the message is not to use them willy nilly. Going back to my patch series, 3/4 is ok, but 4/4 mostly crosses the line. Do I have that right?
It is the exact opposite for me : 3/4 makes all printf functions work like some kind of *nprintf(), while 4/4 is about the network code switching to *nprintf() for safety, so 3/4 would be nak and 4/4 ack as far as I am concerned.
Basically, printf family functions which do not have the 'n' are *know* by all -- experienced enough :) -- programmers to be *unsafe* (but to require less from the caller) and it should remain so: no programmer should ever encounter an implementation of printf that pretends to be even somewhat safe, because it might bite him/her elsewhere, in another project based on another C library where printf is just the beartrap it usually is.
IOW, programmers already have assumptions about *printf(), including how to deal with length limitations and what happens if you don't, and it is best that these assumption remain true whatever project they work with.
By the way, printf() ends up calling the same code, but without limit checking in place. The alternative is to duplicate all the format string processing code (a limit-checking version and an unchecked version) which would be worse.
I don't intend to dictate the way things can be implemented, so the degree of code reuse is an open question as far as I am concerned. I am only voicing my opinion that *printf() APIs and their contracts should remain identical across all implementations of *printf(), and thus that providing *nprintf() where they don't exist is commandable, but hardening printf() is not, since you basically cannot do it without somewhat departing from the de facto standard.
Regards, Simon
Amicalement,
Hi Albert,
On Sun, Sep 25, 2011 at 1:40 AM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Le 24/09/2011 16:00, Simon Glass a écrit :
So basically the choice is between:
- adding code to the printf() family to try and fix an issue that it is
fundamentally unable to properly fix, and for which a solution exists, or
- grepping and fixing calls to *sprintf() in U-Boot that do not respect
the known contraints of printf(), by resizing the buffer or calling *snprintf() instead.
I am definitely not in favor of the first option concerning U-Boot.
Sounds fine to me. So I think we need the nprintf() variants in there, but the message is not to use them willy nilly. Going back to my patch series, 3/4 is ok, but 4/4 mostly crosses the line. Do I have that right?
It is the exact opposite for me : 3/4 makes all printf functions work like some kind of *nprintf(), while 4/4 is about the network code switching to *nprintf() for safety, so 3/4 would be nak and 4/4 ack as far as I am concerned.
Basically, printf family functions which do not have the 'n' are *know* by all -- experienced enough :) -- programmers to be *unsafe* (but to require less from the caller) and it should remain so: no programmer should ever encounter an implementation of printf that pretends to be even somewhat safe, because it might bite him/her elsewhere, in another project based on another C library where printf is just the beartrap it usually is.
IOW, programmers already have assumptions about *printf(), including how to deal with length limitations and what happens if you don't, and it is best that these assumption remain true whatever project they work with.
I don't quite understand this. You are saying that we shouldn't modify the existing printf(), serial_printf() etc. so live within the buffer they use? If I call printf() and my resulting string is too long for the library then I would much prefer to get truncated output than have to hunt for a wierd crash.
It sounds like you are asking for a new printf(), serial_printf() which provides the facility of limiting the output to n characters, and leave these ones alone. But these functions are not passed a buffer - they use their own and they set the site of it. So I think they should be responsible for enforcing that size.
If you are asking for a new set of functions - nprintf(), serial_nprintf(), etc. then I wonder how you can pass the 'n' but not the actual buffer. In my mind you should not do one without the other.
So please can you clarify?
By the way, printf() ends up calling the same code, but without limit checking in place. The alternative is to duplicate all the format string processing code (a limit-checking version and an unchecked version) which would be worse.
I don't intend to dictate the way things can be implemented, so the degree of code reuse is an open question as far as I am concerned. I am only voicing my opinion that *printf() APIs and their contracts should remain identical across all implementations of *printf(), and thus that providing *nprintf() where they don't exist is commandable, but hardening printf() is not, since you basically cannot do it without somewhat departing from the de facto standard.
OK fair enough. People are used to printf() just working, no matter what the size. I haven't looked at the implementation but I suspect when the buffer fills it outputs it and starts the buffer again .In any case you don't have to worry about it with the GNU C library.
We probably don't need to go that far in U-Boot, but some simple checking would avoid a nasty surprise for the user. It is obvious from the result that something is truncated, and we can WARN if that helps.
Regards, Simon
Regards, Simon
Amicalement,
Albert.
Hi Simon,
Le 25/09/2011 16:50, Simon Glass a écrit :
Basically, printf family functions which do not have the 'n' are *know* by all -- experienced enough :) -- programmers to be *unsafe* (but to require less from the caller) and it should remain so: no programmer should ever encounter an implementation of printf that pretends to be even somewhat safe, because it might bite him/her elsewhere, in another project based on another C library where printf is just the beartrap it usually is.
IOW, programmers already have assumptions about *printf(), including how to deal with length limitations and what happens if you don't, and it is best that these assumption remain true whatever project they work with.
I don't quite understand this. You are saying that we shouldn't modify the existing printf(), serial_printf() etc. so live within the buffer they use? If I call printf() and my resulting string is too long for the library then I would much prefer to get truncated output than have to hunt for a wierd crash.
I understand the preference for truncated output, but the output length is primarily in the hands of the caller, through the format stringand possibly also through *nprintf() -- i.e. the caller can only get an output overrun if (s)he does not take the required steps to avoid it.
It sounds like you are asking for a new printf(), serial_printf() which provides the facility of limiting the output to n characters, and leave these ones alone. But these functions are not passed a buffer - they use their own and they set the site of it. So I think they should be responsible for enforcing that size.
The existing contract for printf family functions (think wider than U-Boot) does not enforce this responsibility on them.
If you are asking for a new set of functions - nprintf(), serial_nprintf(), etc. then I wonder how you can pass the 'n' but not the actual buffer. In my mind you should not do one without the other.
So please can you clarify?
I should clarify indeed. My opinion, expressed as a single general rule, is "keep the known semantics of *printf() function family unchanged in U-Boot wrt all other printif implementations around". Thus I think that:
- the printf() function should *not* attempt anything to mitigate length issues beyond what the standard mandates. If a user calls printf(), (s)he is expected to be aware of the risks of overruns, and to take -- if (s)he so decides -- steps to avoid it; besides, the function does not have a way. Conversively, implementers of the printf() function need not consider any specific recovery action with regard to size issues. For instance, why do we have an internal buffer for printf to begin with? The implementation does not need them (and besides, I guess the buffer does not respect the single C99 environmental constraint for printf, that any field should be able to be at least 4095 bytes -- no kidding).
- users who actually wisht to limit outpout ca use either
-- a crafted format string with maximum-sized format specifiers, or
-- a call tp a *nprintf() function into a local buffer of known size followed by a call to fputs().
By the way, printf() ends up calling the same code, but without limit checking in place. The alternative is to duplicate all the format string processing code (a limit-checking version and an unchecked version) which would be worse.
I don't intend to dictate the way things can be implemented, so the degree of code reuse is an open question as far as I am concerned. I am only voicing my opinion that *printf() APIs and their contracts should remain identical across all implementations of *printf(), and thus that providing *nprintf() where they don't exist is commandable, but hardening printf() is not, since you basically cannot do it without somewhat departing from the de facto standard.
OK fair enough. People are used to printf() just working, no matter what the size. I haven't looked at the implementation but I suspect when the buffer fills it outputs it and starts the buffer again .In any case you don't have to worry about it with the GNU C library.
You may also find implementations where the buffer is flushed after each literal part in the format string and after each format specifier output. Or some which emit serial characters as soon as they are produced and use a buffer only for those formats where the ASCII representation cannot be easily constructed left-to-right.
We probably don't need to go that far in U-Boot, but some simple checking would avoid a nasty surprise for the user. It is obvious from the result that something is truncated, and we can WARN if that helps.
A user who does not expect a nasty surprise from calling printf() with a fair chance of overflowing the output buffer deserves the surprise. :)
Regards, Simon
Amicalement,
Hi Albert,
On Mon, Sep 26, 2011 at 4:20 AM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hi Simon,
Le 25/09/2011 16:50, Simon Glass a écrit :
Basically, printf family functions which do not have the 'n' are *know* by all -- experienced enough :) -- programmers to be *unsafe* (but to require less from the caller) and it should remain so: no programmer should ever encounter an implementation of printf that pretends to be even somewhat safe, because it might bite him/her elsewhere, in another project based on another C library where printf is just the beartrap it usually is.
IOW, programmers already have assumptions about *printf(), including how to deal with length limitations and what happens if you don't, and it is best that these assumption remain true whatever project they work with.
I don't quite understand this. You are saying that we shouldn't modify the existing printf(), serial_printf() etc. so live within the buffer they use? If I call printf() and my resulting string is too long for the library then I would much prefer to get truncated output than have to hunt for a wierd crash.
I understand the preference for truncated output, but the output length is primarily in the hands of the caller, through the format stringand possibly also through *nprintf() -- i.e. the caller can only get an output overrun if (s)he does not take the required steps to avoid it.
So we are talking about two things here. I am fine with this one - basically people can call sprintf() or snprintf(). If the former then they should be aware of the potential for overflow, if the latter they should prepare for possible truncation.
It sounds like you are asking for a new printf(), serial_printf() which provides the facility of limiting the output to n characters, and leave these ones alone. But these functions are not passed a buffer - they use their own and they set the site of it. So I think they should be responsible for enforcing that size.
The existing contract for printf family functions (think wider than U-Boot) does not enforce this responsibility on them.
Let me re-word: printf() and serial_printf() should be responsible for working within the buffer that they use. In the 'existing printf() contract' it is no concern of the user what the size of that buffer may be. In U-Boot we have an implementation that works wholly within that buffer, and that buffer is small, so we are exposing our limitation, either by a buffer overflow crash or truncation. I advocate the latter.
If you are asking for a new set of functions - nprintf(), serial_nprintf(), etc. then I wonder how you can pass the 'n' but not the actual buffer. In my mind you should not do one without the other.
So please can you clarify?
I should clarify indeed. My opinion, expressed as a single general rule, is "keep the known semantics of *printf() function family unchanged in U-Boot wrt all other printif implementations around". Thus I think that:
- the printf() function should *not* attempt anything to mitigate length
issues beyond what the standard mandates. If a user calls printf(), (s)he is expected to be aware of the risks of overruns, and to take -- if (s)he so decides -- steps to avoid it; besides, the function does not have a way. Conversively, implementers of the printf() function need not consider any specific recovery action with regard to size issues. For instance, why do we have an internal buffer for printf to begin with? The implementation does not need them (and besides, I guess the buffer does not respect the single C99 environmental constraint for printf, that any field should be able to be at least 4095 bytes -- no kidding).
- users who actually wisht to limit outpout ca use either
-- a crafted format string with maximum-sized format specifiers, or
-- a call tp a *nprintf() function into a local buffer of known size followed by a call to fputs().
I think this last one is clumsy. It could lead to buffers all over U-Boot to work around the printf() limitation. If there is a desire for this (and I'm not saying there is), let's enhance printf() to output bit by bit as it goes (as a separate discussion from this one though). One justification for leaving printf() as it is (but with a size limit instead of a buffer overflow) is that people seldom printf more than a few lines at once.
By the way, printf() ends up calling the same code, but without limit checking in place. The alternative is to duplicate all the format string processing code (a limit-checking version and an unchecked version) which would be worse.
I don't intend to dictate the way things can be implemented, so the degree of code reuse is an open question as far as I am concerned. I am only voicing my opinion that *printf() APIs and their contracts should remain identical across all implementations of *printf(), and thus that providing *nprintf() where they don't exist is commandable, but hardening printf() is not, since you basically cannot do it without somewhat departing from the de facto standard.
OK fair enough. People are used to printf() just working, no matter what the size. I haven't looked at the implementation but I suspect when the buffer fills it outputs it and starts the buffer again .In any case you don't have to worry about it with the GNU C library.
You may also find implementations where the buffer is flushed after each literal part in the format string and after each format specifier output. Or some which emit serial characters as soon as they are produced and use a buffer only for those formats where the ASCII representation cannot be easily constructed left-to-right.
We probably don't need to go that far in U-Boot, but some simple checking would avoid a nasty surprise for the user. It is obvious from the result that something is truncated, and we can WARN if that helps.
A user who does not expect a nasty surprise from calling printf() with a fair chance of overflowing the output buffer deserves the surprise. :)
For sprintf() I agree - this is well understood and people are aware of it. For printf() I am not so sure.
Regards, Simon
Regards, Simon
Amicalement,
Albert.
Dear Simon Glass,
In message CAPnjgZ0y_TWJ4ANEXekGxNpeoJHhkth7hod_3Quu2HVcS=1wHA@mail.gmail.com you wrote:
For sprintf() I agree - this is well understood and people are aware of it. For printf() I am not so sure.
We are a resource limited boot loader. We got for a small footprint, and accept some resulting restrictions, if they are not really severe. THe printf() restriction has not exactly frequently popped up before, so I consider it a non-issue.
Best regards,
Wolfgang Denk
On 09/26/2011 06:20 AM, Albert ARIBAUD wrote:
Hi Simon,
Le 25/09/2011 16:50, Simon Glass a écrit :
Basically, printf family functions which do not have the 'n' are *know* by all -- experienced enough :) -- programmers to be *unsafe* (but to require less from the caller)
printf()/fprintf() are usually safe enough for the things they're used for.
and it should remain so: no programmer should ever
encounter an implementation of printf that pretends to be even somewhat safe, because it might bite him/her elsewhere, in another project based on another C library where printf is just the beartrap it usually is.
And we should mount large spikes on every steering wheel to make people drive more carefully. :-P
I should clarify indeed. My opinion, expressed as a single general rule, is "keep the known semantics of *printf() function family unchanged in U-Boot wrt all other printif implementations around". Thus I think that:
FWIW, Linux's printk() truncates output if it exceeds the internal buffer. glibc's is an ugly mess, but appears to not need a fixed printf buffer -- it streams output into its file buffering mechanism. That there may be some crappy implementations out there is no excuse for making these functions difficult to use everywhere.
Conversively, implementers of the printf() function need not consider any specific recovery action with regard to size issues. For instance, why do we have an internal buffer for printf to begin with? The implementation does not need them
Apparently this implementation does. I suppose you could rewrite the code to not need one, but is that really worthwhile here, compared to this easy and virtually cost-free (once you have snprintf) form of damage control? It's just passing in a meaningful number to vsnprintf instead of INT_MAX. You could save some bytes by removing vsprintf(), as well.
(and besides, I guess the buffer does not respect the single C99 environmental constraint for printf, that any field should be able to be at least 4095 bytes -- no kidding).
That this minimum is more than most boards allow for the entire output, and that it is board-specific (so it could work for one board but fail for another), are all the more reason to want some safety here. I'd much rather see a truncated message (and proceed to fix the obvious bug) than have to debug corruption and possibly recover a bricked board.
- users who actually wisht to limit outpout ca use either
You say "actually wish to limit output" as if "let it corrupt memory if it's too large" is the normal thing to want.
-scott
On 27/09/2011 00:28, Scott Wood wrote:
- users who actually wisht to limit outpout ca use either
You say "actually wish to limit output" as if "let it corrupt memory if it's too large" is the normal thing to want.
What I meant was "users who actually want to limit output explicitly by truncating it rather than implicitly by always outputting small enough text or crafting limitations in the format string specifiers, ..."
-scott
Amicalement,
Hi,
On Mon, Sep 26, 2011 at 11:52 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
On 27/09/2011 00:28, Scott Wood wrote:
- users who actually wisht to limit outpout ca use either
You say "actually wish to limit output" as if "let it corrupt memory if it's too large" is the normal thing to want.
What I meant was "users who actually want to limit output explicitly by truncating it rather than implicitly by always outputting small enough text or crafting limitations in the format string specifiers, ..."
-scott
Amicalement,
Albert.
Just to follow up this thread, I am going to drop the network patch and resubmit the rest of it. This will make snprintf() available in U-Boot.
Regards, Simon
Dear Simon Glass,
In message CAPnjgZ3hYmXEHr8MT0deZ054kuOxkNDt4Sa+nVPSzPAdAQFf0w@mail.gmail.com you wrote:
Just to follow up this thread, I am going to drop the network patch and resubmit the rest of it. This will make snprintf() available in U-Boot.
Without code that uses it? We don't accept dead code...
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Mon, Oct 10, 2011 at 1:36 PM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message CAPnjgZ3hYmXEHr8MT0deZ054kuOxkNDt4Sa+nVPSzPAdAQFf0w@mail.gmail.com you wrote:
Just to follow up this thread, I am going to drop the network patch and resubmit the rest of it. This will make snprintf() available in U-Boot.
Without code that uses it? We don't accept dead code...
It is used by printf() and the like - please see console.c.
Regards, Simon
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Due to lack of disk space, this fortune database has been discontinued.
Dear Simon Glass,
In message 1316799532-20761-1-git-send-email-sjg@chromium.org you wrote:
The printf family of functions in U-Boot cannot deal with a situation where the caller provides a buffer which turns out to be too small for the format string. This can result in buffer overflows, stack overflows and other bad behavior.
This patch series tidies this up in the common vsprintf.c code, and also some network code (as an example of where this might lead).
What's the impact of this patch set on the memory footprint of typical configurations?
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Sun, Sep 25, 2011 at 1:04 PM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message 1316799532-20761-1-git-send-email-sjg@chromium.org you wrote:
The printf family of functions in U-Boot cannot deal with a situation where the caller provides a buffer which turns out to be too small for the format string. This can result in buffer overflows, stack overflows and other bad behavior.
This patch series tidies this up in the common vsprintf.c code, and also some network code (as an example of where this might lead).
What's the impact of this patch set on the memory footprint of typical configurations?
Good question. The short answer with my ARMv7 compiler (gcc-4.4.3_cos_gg_53174) is 328 bytes, about 10% increase to code size vsprintf.o.
The newly added functions (snprintf, vscnprintf, scnprintf) are a total of 116 bytes.
The changes to number(), string() and vsprintf() to make them respect an end pointer increase size by 80, 20 and 80 bytes respectively.
Total text size for existing vsprintf.o functions goes from 0xc10 (3088) to 0xd58 (3416), or 328 bytes. Of this 116 bytes is the new functions and the rest is dealing with the end pointer. There is no data.
Regards, Simon
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Nail here --X-- for new monitor.
participants (8)
-
Albert ARIBAUD -
Graeme Russ -
Kumar Gala -
Mike Frysinger -
Scott Wood -
Simon Glass -
Sonny Rao -
Wolfgang Denk