From patchwork Sat Feb 2 22:17:56 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 1035523 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=sourceware.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=libc-alpha-return-99704-incoming=patchwork.ozlabs.org@sourceware.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="LMw1SGlq"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 43sSz45Nv4z9s4V for ; Sun, 3 Feb 2019 09:18:08 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:date:message-id:mime-version :content-type; q=dns; s=default; b=Gba1Kzy2T63+79WdQjlNKDso/M5PC bQKDgpgbDZ4J6JfVf6Pno0g43sDDuhjH2pDqup0Ab1BkuGhvV1suLSto1RH6iS4o akziV/+nijMbxkEPd0gDdXUcMVx6ljzyp4swtnGf7XESh/+l493+k21ruWLH38Pa 2tQ+bCoPI+HQn8= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:date:message-id:mime-version :content-type; s=default; bh=8tAuR3l0g3Qf5pwdF51hZxrOSIQ=; b=LMw 1SGlquw0AWUyjnn4/wXPnaMH32Hs6zVROCQWMNv0jO6/YelBVfXiHCUJe+zWzyOZ lK3Ve6mv1xjmCmw2McCTzDahTlxxm0ZE2jPBzK+9f/vVuQKQkEvoAJSk1tf4YNLL jKybI5EE89vlu+9kAnwUClxCrr2nswbpRTiw+Z2I= Received: (qmail 66138 invoked by alias); 2 Feb 2019 22:18:02 -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 66126 invoked by uid 89); 2 Feb 2019 22:18:01 -0000 Authentication-Results: sourceware.org; auth=none 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, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=lose X-HELO: mx1.redhat.com From: Florian Weimer To: libc-alpha@sourceware.org Subject: [PATCH] time: Use struct alloc_buffer in __tzfile_read Date: Sat, 02 Feb 2019 23:17:56 +0100 Message-ID: <87womh7s2z.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 This simplification makes it easier to rearrange the allocations to avoid alignment gaps in a future gap. The computation of tzspec_len is moved in front of the total_size computation, so that the allocation size computation and the suballocations are next to each other. Also add an assert that tzspec_len is positive when it is actually used later. 2019-02-02 Florian Weimer * time/tzfile.c (__tzfile_read): Use struct alloc_buffer and its implicit overflow checks. diff --git a/time/tzfile.c b/time/tzfile.c index 24440a6a9c..72adfbc68e 100644 --- a/time/tzfile.c +++ b/time/tzfile.c @@ -25,6 +25,7 @@ #include #include #include +#include #include @@ -105,12 +106,8 @@ __tzfile_read (const char *file, size_t extra, char **extrap) struct tzhead tzhead; size_t chars; size_t i; - size_t total_size; - size_t types_idx; - size_t leaps_idx; int was_using_tzfile = __use_tzfile; int trans_width = 4; - size_t tzspec_len; char *new = NULL; _Static_assert (sizeof (__time64_t) == 8, @@ -215,32 +212,9 @@ __tzfile_read (const char *file, size_t extra, char **extrap) goto read_again; } - if (__builtin_expect (num_transitions - > ((SIZE_MAX - (__alignof__ (struct ttinfo) - 1)) - / (sizeof (__time64_t) + 1)), 0)) - goto lose; - total_size = num_transitions * (sizeof (__time64_t) + 1); - total_size = ((total_size + __alignof__ (struct ttinfo) - 1) - & ~(__alignof__ (struct ttinfo) - 1)); - types_idx = total_size; - if (__builtin_expect (num_types - > (SIZE_MAX - total_size) / sizeof (struct ttinfo), 0)) - goto lose; - total_size += num_types * sizeof (struct ttinfo); - if (__glibc_unlikely (chars > SIZE_MAX - total_size)) - goto lose; - total_size += chars; - if (__builtin_expect (__alignof__ (struct leap) - 1 - > SIZE_MAX - total_size, 0)) - goto lose; - total_size = ((total_size + __alignof__ (struct leap) - 1) - & ~(__alignof__ (struct leap) - 1)); - leaps_idx = total_size; - if (__builtin_expect (num_leaps - > (SIZE_MAX - total_size) / sizeof (struct leap), 0)) - goto lose; - total_size += num_leaps * sizeof (struct leap); - tzspec_len = 0; + /* Compute the size of the POSIX time zone specification in the + file. */ + size_t tzspec_len; if (trans_width == 8) { off_t rem = st.st_size - __ftello (f); @@ -262,30 +236,59 @@ __tzfile_read (const char *file, size_t extra, char **extrap) if (__glibc_unlikely (tzspec_len == 0 || tzspec_len - 1 < num_isgmt)) goto lose; tzspec_len -= num_isgmt + 1; - if (__glibc_unlikely (tzspec_len == 0 - || SIZE_MAX - total_size < tzspec_len)) + if (tzspec_len == 0) goto lose; } - if (__glibc_unlikely (SIZE_MAX - total_size - tzspec_len < extra)) - goto lose; - - /* Allocate enough memory including the extra block requested by the - caller. */ - transitions = malloc (total_size + tzspec_len + extra); - if (transitions == NULL) - goto lose; - - type_idxs = (unsigned char *) transitions + (num_transitions - * sizeof (__time64_t)); - types = (struct ttinfo *) ((char *) transitions + types_idx); - zone_names = (char *) types + num_types * sizeof (struct ttinfo); - leaps = (struct leap *) ((char *) transitions + leaps_idx); - if (trans_width == 8) - tzspec = (char *) leaps + num_leaps * sizeof (struct leap) + extra; else - tzspec = NULL; + tzspec_len = 0; + + /* The file is parsed into a single heap allocation, comprising of + the following arrays: + + __time64_t transitions[num_transitions]; + struct ttinfo types[num_types]; + unsigned char type_idxs[num_types]; + char zone_names[chars]; + struct leap leaps[num_leaps]; + char extra_array[extra]; // Stored into *pextras if requested. + char tzspec[tzspec_len]; + + The piece-wise allocations from buf below verify that no + overflow/wraparound occurred in these computations. */ + struct alloc_buffer buf; + { + size_t total_size = num_transitions * (sizeof (__time64_t) + 1); + total_size = ((total_size + __alignof__ (struct ttinfo) - 1) + & ~(__alignof__ (struct ttinfo) - 1)); + total_size += num_types * sizeof (struct ttinfo) + chars; + total_size = ((total_size + __alignof__ (struct leap) - 1) + & ~(__alignof__ (struct leap) - 1)); + total_size += num_leaps * sizeof (struct leap) + tzspec_len + extra; + + transitions = malloc (total_size); + if (transitions == NULL) + goto lose; + buf = alloc_buffer_create (transitions, total_size); + } + + if (alloc_buffer_alloc_array (&buf, __time64_t, num_transitions) + != transitions) + /* Either the start of the allocation moved unexpectedly + (misaligned heap pointer, should not happen), or we had an + overflow. */ + goto lose; + type_idxs = alloc_buffer_alloc_array (&buf, unsigned char, num_transitions); + types = alloc_buffer_alloc_array (&buf, struct ttinfo, num_types); + zone_names = alloc_buffer_alloc_array (&buf, char, chars); + leaps = alloc_buffer_alloc_array (&buf, struct leap, num_leaps); if (extra > 0) - *extrap = (char *) &leaps[num_leaps]; + *extrap = alloc_buffer_alloc_array (&buf, char, extra); + if (trans_width == 8) + tzspec = alloc_buffer_alloc_array (&buf, char, tzspec_len); + else + tzspec = NULL; + if (alloc_buffer_has_failed (&buf)) + goto lose; if (__builtin_expect (trans_width == 8, 1)) { @@ -390,6 +393,7 @@ __tzfile_read (const char *file, size_t extra, char **extrap) /* Read the POSIX TZ-style information if possible. */ if (tzspec != NULL) { + assert (tzspec_len > 0); /* Skip over the newline first. */ if (__getc_unlocked (f) != '\n' || (__fread_unlocked (tzspec, 1, tzspec_len - 1, f)