Re: [U-Boot] [PATCH 1/3] vsprintf.c: Always enable CONFIG_SYS_VSNPRINTF

Hi Tom,

On Thu, Jan 14, 2016 at 01:02:03PM -0500, Tom Rini wrote:

Enabling this function always removes some class of string saftey issues. The size change here in general is about 400 bytes and this seems a reasonable trade-off.

Cc: Peng Fan peng.fan@nxp.com Cc: Peter Robinson pbrobinson@gmail.com Cc: Fabio Estevam fabio.estevam@freescale.com Cc: Adrian Alonso aalonso@freescale.com Cc: Stefano Babic sbabic@denx.de Cc: Hans de Goede hdegoede@redhat.com Signed-off-by: Tom Rini trini@konsulko.com

Reviewed-and-tested-by: Peng Fan peng.fan@nxp.com

Regards, Peng.

---

I ran my usual world build of buildman with size comparison options this time to test this out. The only exception to the general size change here are q8_a23_tablet_800x480 and gt90h_v4 both in the non-SPL case. These both grow by 3780 bytes in the non-SPL case. In all cases except those two, when paired with the gzwrite patch I posted yesterday we gain all of that back, or more.

---

README | 9 --------- configs/bayleybay_defconfig | 1 - configs/chromebook_link_defconfig | 1 - configs/chromebox_panther_defconfig | 1 - configs/coreboot-x86_defconfig | 1 - configs/crownbay_defconfig | 1 - configs/galileo_defconfig | 1 - configs/minnowmax_defconfig | 1 - configs/qemu-x86_defconfig | 1 - configs/sandbox_defconfig | 1 - include/vsprintf.h | 12 ------------ lib/Kconfig | 9 --------- lib/vsprintf.c | 12 ------------ 13 files changed, 51 deletions(-)

diff --git a/README b/README index 5ac2d44..b7ce63d 100644 --- a/README +++ b/README @@ -890,15 +890,6 @@ The following options need to be configured: 'Sane' compilers will generate smaller code if CONFIG_PRE_CON_BUF_SZ is a power of 2

-- Safe printf() functions

• Define CONFIG_SYS_VSNPRINTF to compile in safe versions of
  

• the printf() functions. These are defined in
  

• include/vsprintf.h and include snprintf(), vsnprintf() and
  

• so on. Code size increase is approximately 300-500 bytes.
  

• If this option is not given then these functions will
  

• silently discard their buffer size argument - this means
  

• you are not getting any overflow checking in this case.
  

• Boot Delay: CONFIG_BOOTDELAY - in seconds Delay before automatically booting the default image; set to -1 to disable autoboot.

diff --git a/configs/bayleybay_defconfig b/configs/bayleybay_defconfig index f462e05..0879d1e 100644 --- a/configs/bayleybay_defconfig +++ b/configs/bayleybay_defconfig @@ -37,4 +37,3 @@ CONFIG_VIDEO_VESA=y CONFIG_FRAMEBUFFER_SET_VESA_MODE=y CONFIG_FRAMEBUFFER_VESA_MODE_11A=y CONFIG_USE_PRIVATE_LIBGCC=y -CONFIG_SYS_VSNPRINTF=y diff --git a/configs/chromebook_link_defconfig b/configs/chromebook_link_defconfig index dbfbb97..baa0ed8 100644 --- a/configs/chromebook_link_defconfig +++ b/configs/chromebook_link_defconfig @@ -38,5 +38,4 @@ CONFIG_VIDEO_VESA=y CONFIG_FRAMEBUFFER_SET_VESA_MODE=y CONFIG_FRAMEBUFFER_VESA_MODE_11A=y CONFIG_USE_PRIVATE_LIBGCC=y -CONFIG_SYS_VSNPRINTF=y CONFIG_TPM=y diff --git a/configs/chromebox_panther_defconfig b/configs/chromebox_panther_defconfig index ed4428f..c368cc0 100644 --- a/configs/chromebox_panther_defconfig +++ b/configs/chromebox_panther_defconfig @@ -33,5 +33,4 @@ CONFIG_VIDEO_VESA=y CONFIG_FRAMEBUFFER_SET_VESA_MODE=y CONFIG_FRAMEBUFFER_VESA_MODE_11A=y CONFIG_USE_PRIVATE_LIBGCC=y -CONFIG_SYS_VSNPRINTF=y CONFIG_TPM=y diff --git a/configs/coreboot-x86_defconfig b/configs/coreboot-x86_defconfig index cd2be18..fda0db2 100644 --- a/configs/coreboot-x86_defconfig +++ b/configs/coreboot-x86_defconfig @@ -25,5 +25,4 @@ CONFIG_TPM_TIS_LPC=y CONFIG_USB=y CONFIG_DM_USB=y CONFIG_USE_PRIVATE_LIBGCC=y -CONFIG_SYS_VSNPRINTF=y CONFIG_TPM=y diff --git a/configs/crownbay_defconfig b/configs/crownbay_defconfig index 932d9ec..6bc4b8d 100644 --- a/configs/crownbay_defconfig +++ b/configs/crownbay_defconfig @@ -36,4 +36,3 @@ CONFIG_DM_USB=y CONFIG_VIDEO_VESA=y CONFIG_FRAMEBUFFER_SET_VESA_MODE=y CONFIG_USE_PRIVATE_LIBGCC=y -CONFIG_SYS_VSNPRINTF=y diff --git a/configs/galileo_defconfig b/configs/galileo_defconfig index 0604aa7..925d3ee 100644 --- a/configs/galileo_defconfig +++ b/configs/galileo_defconfig @@ -28,4 +28,3 @@ CONFIG_TIMER=y CONFIG_USB=y CONFIG_DM_USB=y CONFIG_USE_PRIVATE_LIBGCC=y -CONFIG_SYS_VSNPRINTF=y diff --git a/configs/minnowmax_defconfig b/configs/minnowmax_defconfig index 864fd1b..af6a8ec 100644 --- a/configs/minnowmax_defconfig +++ b/configs/minnowmax_defconfig @@ -39,4 +39,3 @@ CONFIG_VIDEO_VESA=y CONFIG_FRAMEBUFFER_SET_VESA_MODE=y CONFIG_FRAMEBUFFER_VESA_MODE_11A=y CONFIG_USE_PRIVATE_LIBGCC=y -CONFIG_SYS_VSNPRINTF=y diff --git a/configs/qemu-x86_defconfig b/configs/qemu-x86_defconfig index 8c86931..b0c935c 100644 --- a/configs/qemu-x86_defconfig +++ b/configs/qemu-x86_defconfig @@ -30,4 +30,3 @@ CONFIG_VIDEO_VESA=y CONFIG_FRAMEBUFFER_SET_VESA_MODE=y CONFIG_FRAMEBUFFER_VESA_MODE_111=y CONFIG_USE_PRIVATE_LIBGCC=y -CONFIG_SYS_VSNPRINTF=y diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 731fc25..caa7336 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -76,7 +76,6 @@ CONFIG_USB_EMUL=y CONFIG_USB_STORAGE=y CONFIG_USB_KEYBOARD=y CONFIG_SYS_USB_EVENT_POLL=y -CONFIG_SYS_VSNPRINTF=y CONFIG_CMD_DHRYSTONE=y CONFIG_TPM=y CONFIG_LZ4=y diff --git a/include/vsprintf.h b/include/vsprintf.h index b5bc9c1..376f5dd 100644 --- a/include/vsprintf.h +++ b/include/vsprintf.h @@ -124,7 +124,6 @@ int sprintf(char *buf, const char *fmt, ...) int vsprintf(char *buf, const char *fmt, va_list args); char *simple_itoa(ulong i);

-#ifdef CONFIG_SYS_VSNPRINTF /**

• Format a string and place it in a buffer

@@ -199,17 +198,6 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args);

• See the vsprintf() documentation for format string extensions over C99.

*/ int vscnprintf(char *buf, size_t size, const char *fmt, va_list args); -#else -/*

• • Use macros to silently drop the size parameter. Note that the 'cn'
• • versions are the same as the 'n' versions since the functions assume
• • there is always enough buffer space when !CONFIG_SYS_VSNPRINTF
• */

-#define snprintf(buf, size, fmt, args...) sprintf(buf, fmt, ##args) -#define scnprintf(buf, size, fmt, args...) sprintf(buf, fmt, ##args) -#define vsnprintf(buf, size, fmt, args...) vsprintf(buf, fmt, ##args) -#define vscnprintf(buf, size, fmt, args...) vsprintf(buf, fmt, ##args) -#endif /* CONFIG_SYS_VSNPRINTF */

/**

• print_grouped_ull() - print a value with digits grouped by ','

diff --git a/lib/Kconfig b/lib/Kconfig index 9d580e4..46d7034 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -27,15 +27,6 @@ config SYS_HZ get_timer() must operate in milliseconds and this option must be set to 1000.

-config SYS_VSNPRINTF

• bool "Enable safe version of sprintf()"
• help

•  Since sprintf() can overflow its buffer, it is common to use
  

•  snprintf() instead, which knows the buffer size and can avoid
  

•  overflow. However, this does increase code size slightly (for
  

•  Thumb-2, about 420 bytes). Enable this option for safety when
  

•  using sprintf() with data you do not control.
  

config USE_TINY_PRINTF bool "Enable tiny printf() version" help diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 24167a1..874a295 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -141,7 +141,6 @@ static noinline char *put_dec(char *buf, uint64_t num) #define SMALL 32 /* Must be 32 == 0x20 */ #define SPECIAL 64 /* 0x */

-#ifdef CONFIG_SYS_VSNPRINTF /*

• Macro to add a new character to our output string, but only if it will
• fit. The macro moves to the next character position in the output string.

@@ -151,9 +150,6 @@ static noinline char *put_dec(char *buf, uint64_t num) *(str) = (ch); \ ++str; \ } while (0) -#else -#define ADDCH(str, ch) (*(str)++ = (ch)) -#endif

static char *number(char *buf, char *end, u64 num, int base, int size, int precision, int type) @@ -441,13 +437,11 @@ static int vsnprintf_internal(char *buf, size_t size, const char *fmt, /* 't' added for ptrdiff_t */ char *end = buf + size;

-#ifdef CONFIG_SYS_VSNPRINTF /* Make sure end is always >= buf - do we want this in U-Boot? */ if (end < buf) { end = ((void *)-1); size = end - buf; } -#endif str = buf;

for (; *fmt ; ++fmt) { @@ -609,21 +603,16 @@ repeat: flags); }

-#ifdef CONFIG_SYS_VSNPRINTF if (size > 0) { ADDCH(str, '\0'); if (str > end) end[-1] = '\0'; --str; } -#else

• *str = '\0';

-#endif /* the trailing null byte doesn't count towards the total */ return str - buf; }

-#ifdef CONFIG_SYS_VSNPRINTF int vsnprintf(char *buf, size_t size, const char *fmt, va_list args) { @@ -666,7 +655,6 @@ int scnprintf(char *buf, size_t size, const char *fmt, ...)

return i; } -#endif /* CONFIG_SYS_VSNPRINT */

/**

• Format a string and place it in a buffer (va_list version)

-- 2.6.4

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot