From patchwork Mon Feb 2 19:23:29 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paul Eggert X-Patchwork-Id: 4885 Received: (qmail 24633 invoked by alias); 2 Feb 2015 19:23:35 -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 24621 invoked by uid 89); 2 Feb 2015 19:23:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=AWL, BAYES_00, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: smtp.cs.ucla.edu Message-ID: <54CFCEB1.8090301@cs.ucla.edu> Date: Mon, 02 Feb 2015 11:23:29 -0800 From: Paul Eggert User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Paul Pluzhnikov , Andreas Schwab CC: Rich Felker , libc-alpha@sourceware.org Subject: Re: [patch] Fix for heap overflow in wscanf (BZ 16618) References: <20150202050906.GF23507@brightrain.aerifal.cx> In-Reply-To: On 02/02/2015 08:57 AM, Paul Pluzhnikov wrote: > + if (__glibc_unlikely (wpmax > SIZE_MAX / 2) \ > + || __glibc_unlikely (newsize > SIZE_MAX / sizeof (CHAR_T))) \ "if (__glibc_unlikely (wpmax > SIZE_MAX / sizeof (CHAR_T) / 2)" is shorter and faster. Also, the proposed patch duplicates the use_malloc / free / done code, which is messy. And there's an existing use of 'use_malloc' which makes sense only if the code ignores overflow, which it no longer does. So, how about the attached (untested) patch to vfscanf.c instead? It's simpler. It does rely on realloc (wp, SIZE_MAX) returning NULL, but that's safe in glibc. diff --git a/stdio-common/vfscanf.c b/stdio-common/vfscanf.c index cd129a8..0e204e7 100644 --- a/stdio-common/vfscanf.c +++ b/stdio-common/vfscanf.c @@ -272,9 +272,10 @@ _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr, if (__glibc_unlikely (wpsize == wpmax)) \ { \ CHAR_T *old = wp; \ - size_t newsize = (UCHAR_MAX + 1 > 2 * wpmax \ - ? UCHAR_MAX + 1 : 2 * wpmax); \ - if (use_malloc || !__libc_use_alloca (newsize)) \ + bool fits = __glibc_likely (wpmax <= SIZE_MAX / sizeof (CHAR_T) / 2); \ + size_t wpneed = MAX (UCHAR_MAX + 1, 2 * wpmax); \ + size_t newsize = fits ? wpneed * sizeof (CHAR_T) : SIZE_MAX; \ + if (!__libc_use_alloca (newsize)) \ { \ wp = realloc (use_malloc ? wp : NULL, newsize); \ if (wp == NULL) \ @@ -286,14 +287,13 @@ _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr, } \ if (! use_malloc) \ MEMCPY (wp, old, wpsize); \ - wpmax = newsize; \ + wpmax = wpneed; \ use_malloc = true; \ } \ else \ { \ size_t s = wpmax * sizeof (CHAR_T); \ - wp = (CHAR_T *) extend_alloca (wp, s, \ - newsize * sizeof (CHAR_T)); \ + wp = (CHAR_T *) extend_alloca (wp, s, newsize); \ wpmax = s / sizeof (CHAR_T); \ if (old != NULL) \ MEMCPY (wp, old, wpsize); \