From patchwork Tue May 30 08:56:05 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 20636 Received: (qmail 65103 invoked by alias); 30 May 2017 08:56:10 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 64731 invoked by uid 89); 30 May 2017 08:56:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=24h, 3225, JUMP, fweimer@redhat.com X-HELO: mx1.redhat.com DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 06A2881240 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=fweimer@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 06A2881240 Subject: Re: [PATCH] stdio-common/vfprintf.c: Remove magic number 32. To: Carlos O'Donell References: <5c0d8636-3b72-bd94-4fea-61d84d9d254f@redhat.com> Cc: libc-alpha@sourceware.org From: Florian Weimer Message-ID: Date: Tue, 30 May 2017 10:56:05 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 In-Reply-To: <5c0d8636-3b72-bd94-4fea-61d84d9d254f@redhat.com> On 05/04/2017 06:26 PM, Carlos O'Donell wrote: > I was looking at modifying some of this code for a bug I was working > on and it was super annoying that the 32 character buffer is hardcoded > everywhere. This patch simply hoists this into a macro EXTSIZ. > > I'll check this in within 24h if nobody objects. > > 2017-05-04 Carlos O'Donell > > * stdio-common/vfprintf.c (EXTSIZ): Define. > (vfprintf): Use EXTSIZ. > (printf_positional): Likewise. I looked at this and discovered that since 1999, we no longer use the work buffer for padding purposes, so resizing it based on field width is no longer needed. The attached patch removes the resizing logic. We could probably reduce the size of the work buffer, too, and we should have some checks that a funny locale does not produce number formatting rules which exceed the buffer size. (Floating point formatting does not use the work buffer.) Thanks, Florian vfprintf: Do not resize work buffer based on width/precision Since commit 3e95f6602b226e0de06aaff686dc47b282d7cc16 (Remove limitation on size of precision for integers), padding is no longer written to the work buffer, only the number itself (and its transformations), so a statically sized buffer is sufficient. This commit does not remove existing overflow checks which lead to early failures, but it adjusts them not to use EXTSIZE. This causes one of the EOVERFLOW tests in stdio-comment/bug22 to pass. This commit adjust it so that it fails again. 2017-05-30 Florian Weimer * stdio-common/vfprintf.c (EXTSIZ): Remove definition. (WORK_BUFFER_SIZE): Expand comment. (vfprintf): Remove workstart. Turn workend into a constant. Remove work buffer resizing. (printf_positional): Likewise. * stdio-common/bug22.c: Convert to . (do_test): Adjust SN3 test to trigger the overflow again. diff --git a/stdio-common/bug22.c b/stdio-common/bug22.c index b26399a..38af20c 100644 --- a/stdio-common/bug22.c +++ b/stdio-common/bug22.c @@ -1,6 +1,7 @@ /* BZ #5424 */ #include #include +#include /* INT_MAX + 1 */ #define N 2147483648 @@ -32,25 +33,26 @@ do_test (void) ret = fprintf (fp, "%" SN "d", 1); printf ("ret = %d\n", ret); - if (ret != -1 || errno != EOVERFLOW) - return 1; + TEST_VERIFY (ret == -1); + TEST_VERIFY (errno == EOVERFLOW); ret = fprintf (fp, "%." SN "d", 1); printf ("ret = %d\n", ret); - if (ret != -1 || errno != EOVERFLOW) - return 1; + TEST_VERIFY (ret == -1); + TEST_VERIFY (errno == EOVERFLOW); - ret = fprintf (fp, "%." SN3 "d", 1); + ret = fprintf (fp, "%." SN3 "d%4d", 1, 2); printf ("ret = %d\n", ret); - if (ret != -1 || errno != EOVERFLOW) - return 1; + TEST_VERIFY (ret == -1); + TEST_VERIFY (errno == EOVERFLOW); ret = fprintf (fp, "%" SN2 "d%" SN2 "d", 1, 1); printf ("ret = %d\n", ret); + TEST_VERIFY (ret == -1); + TEST_VERIFY (errno == EOVERFLOW); - return ret != -1 || errno != EOVERFLOW; + return 0; } #define TIMEOUT 60 -#define TEST_FUNCTION do_test () -#include "../test-skeleton.c" +#include diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c index 2cf7c8a..b179b20 100644 --- a/stdio-common/vfprintf.c +++ b/stdio-common/vfprintf.c @@ -45,10 +45,6 @@ #define va_list _IO_va_list #undef BUFSIZ #define BUFSIZ _IO_BUFSIZ -/* In some cases we need extra space for all the output which is not - counted in the width of the string. We assume 32 characters is - enough. */ -#define EXTSIZ 32 #define ARGCHECK(S, Format) \ do \ { \ @@ -204,7 +200,9 @@ typedef wchar_t THOUSANDS_SEP_T; /* Global constants. */ static const CHAR_T null[] = L_("(null)"); -/* Size of the work_buffer variable (in characters, not bytes. */ +/* Size of the work_buffer variable (in characters, not bytes. This + must be large enough to store all formatted integers, including + grouping and alternative digits, but excluding padding. */ enum { WORK_BUFFER_SIZE = 1000 }; /* This table maps a character into a number representing a class. In @@ -1258,8 +1256,7 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap) /* Buffer intermediate results. */ CHAR_T work_buffer[WORK_BUFFER_SIZE]; - CHAR_T *workstart = NULL; - CHAR_T *workend; + CHAR_T *const workend = work_buffer + WORK_BUFFER_SIZE; /* We have to save the original argument pointer. */ va_list ap_save; @@ -1367,9 +1364,6 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap) UCHAR_T pad = L_(' ');/* Padding character. */ CHAR_T spec; - workstart = NULL; - workend = work_buffer + WORK_BUFFER_SIZE; - /* Get current character in format string. */ JUMP (*++f, step0_jumps); @@ -1460,62 +1454,25 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap) left = 1; } - if (__glibc_unlikely (width >= INT_MAX / sizeof (CHAR_T) - EXTSIZ)) + if (__glibc_unlikely (width >= INT_MAX / sizeof (CHAR_T))) { __set_errno (EOVERFLOW); done = -1; goto all_done; } - - if (width >= WORK_BUFFER_SIZE - EXTSIZ) - { - /* We have to use a special buffer. */ - size_t needed = ((size_t) width + EXTSIZ) * sizeof (CHAR_T); - if (__libc_use_alloca (needed)) - workend = (CHAR_T *) alloca (needed) + width + EXTSIZ; - else - { - workstart = (CHAR_T *) malloc (needed); - if (workstart == NULL) - { - done = -1; - goto all_done; - } - workend = workstart + width + EXTSIZ; - } - } } JUMP (*f, step1_jumps); /* Given width in format string. */ LABEL (width): width = read_int (&f); - - if (__glibc_unlikely (width == -1 - || width >= INT_MAX / sizeof (CHAR_T) - EXTSIZ)) + if (__glibc_unlikely (width == -1 || width >= INT_MAX / sizeof (CHAR_T))) { __set_errno (EOVERFLOW); done = -1; goto all_done; } - if (width >= WORK_BUFFER_SIZE - EXTSIZ) - { - /* We have to use a special buffer. */ - size_t needed = ((size_t) width + EXTSIZ) * sizeof (CHAR_T); - if (__libc_use_alloca (needed)) - workend = (CHAR_T *) alloca (needed) + width + EXTSIZ; - else - { - workstart = (CHAR_T *) malloc (needed); - if (workstart == NULL) - { - done = -1; - goto all_done; - } - workend = workstart + width + EXTSIZ; - } - } if (*f == L_('$')) /* Oh, oh. The argument comes from a positional parameter. */ goto do_positional; @@ -1564,34 +1521,6 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap) } else prec = 0; - if (prec > width && prec > WORK_BUFFER_SIZE - EXTSIZ) - { - /* Deallocate any previously allocated buffer because it is - too small. */ - if (__glibc_unlikely (workstart != NULL)) - free (workstart); - workstart = NULL; - if (__glibc_unlikely (prec >= INT_MAX / sizeof (CHAR_T) - EXTSIZ)) - { - __set_errno (EOVERFLOW); - done = -1; - goto all_done; - } - size_t needed = ((size_t) prec + EXTSIZ) * sizeof (CHAR_T); - - if (__libc_use_alloca (needed)) - workend = (CHAR_T *) alloca (needed) + prec + EXTSIZ; - else - { - workstart = (CHAR_T *) malloc (needed); - if (workstart == NULL) - { - done = -1; - goto all_done; - } - workend = workstart + prec + EXTSIZ; - } - } JUMP (*f, step2_jumps); /* Process 'h' modifier. There might another 'h' following. */ @@ -1655,10 +1584,6 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap) /* The format is correctly handled. */ ++nspecs_done; - if (__glibc_unlikely (workstart != NULL)) - free (workstart); - workstart = NULL; - /* Look for next format specifier. */ #ifdef COMPILE_WPRINTF f = __find_specwc ((end_of_spec = ++f)); @@ -1676,18 +1601,11 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap) /* Hand off processing for positional parameters. */ do_positional: - if (__glibc_unlikely (workstart != NULL)) - { - free (workstart); - workstart = NULL; - } done = printf_positional (s, format, readonly_format, ap, &ap_save, done, nspecs_done, lead_str_end, work_buffer, save_errno, grouping, thousands_sep); all_done: - if (__glibc_unlikely (workstart != NULL)) - free (workstart); /* Unlock the stream. */ _IO_funlockfile (s); _IO_cleanup_region_end (0); @@ -1732,8 +1650,6 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format, /* Just a counter. */ size_t cnt; - CHAR_T *workstart = NULL; - if (grouping == (const char *) -1) { #ifdef COMPILE_WPRINTF @@ -1931,8 +1847,7 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format, char pad = specs[nspecs_done].info.pad; CHAR_T spec = specs[nspecs_done].info.spec; - workstart = NULL; - CHAR_T *workend = work_buffer + WORK_BUFFER_SIZE; + CHAR_T *const workend = work_buffer + WORK_BUFFER_SIZE; /* Fill in last information. */ if (specs[nspecs_done].width_arg != -1) @@ -1965,27 +1880,6 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format, prec = specs[nspecs_done].info.prec; } - /* Maybe the buffer is too small. */ - if (MAX (prec, width) + EXTSIZ > WORK_BUFFER_SIZE) - { - if (__libc_use_alloca ((MAX (prec, width) + EXTSIZ) - * sizeof (CHAR_T))) - workend = ((CHAR_T *) alloca ((MAX (prec, width) + EXTSIZ) - * sizeof (CHAR_T)) - + (MAX (prec, width) + EXTSIZ)); - else - { - workstart = (CHAR_T *) malloc ((MAX (prec, width) + EXTSIZ) - * sizeof (CHAR_T)); - if (workstart == NULL) - { - done = -1; - goto all_done; - } - workend = workstart + (MAX (prec, width) + EXTSIZ); - } - } - /* Process format specifiers. */ while (1) { @@ -2059,10 +1953,6 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format, break; } - if (__glibc_unlikely (workstart != NULL)) - free (workstart); - workstart = NULL; - /* Write the following constant string. */ outstring (specs[nspecs_done].end_of_fmt, specs[nspecs_done].next_fmt @@ -2071,8 +1961,6 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format, all_done: if (__glibc_unlikely (args_malloced != NULL)) free (args_malloced); - if (__glibc_unlikely (workstart != NULL)) - free (workstart); scratch_buffer_free (&specsbuf); return done; }