From patchwork Mon Oct 29 23:26:38 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Albert ARIBAUD (3ADEV)" X-Patchwork-Id: 990616 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-96833-incoming=patchwork.ozlabs.org@sourceware.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=3adev.fr Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="pGwEM05B"; 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 42kW2m6HMFz9s3l for ; Tue, 30 Oct 2018 10:26:56 +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:cc:subject:date:message-id; q=dns; s= default; b=W2IZMeQhXbgAAFXh5Dq9kvW5Mv1k1OoWt64JnavtCG5EjsjXaQZsD piTKjJ3YjiTsuzP4pCnkVvjaT6voTv96yTmj4G+1/nq6sHQIJtx4cR5vB46L01D9 CPoGnuQ/DPDZDShP4SItgnjcBMRxJ6uTtTQJd5FWb33VRJbF8DSdrM= 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:cc:subject:date:message-id; s=default; bh=BEYHqHCMxrit5EXGZBgWnhxKFeo=; b=pGwEM05BuP+0GetI8lQQCyl74WDr dm1Ww/Of809Pi0+K+L3DO9q2q8bQhMaDo6lptF94vv6NVeWmLhz1pX0vRnZiwveg x4IJFwMovBbM5WZMiedcj+02c2iu6BoUMBKvLxQzsIfxMOgWxjFMZEf369Lvo9uS JOEvxH9Qg2rDKWw= Received: (qmail 2779 invoked by alias); 29 Oct 2018 23:26:49 -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 2759 invoked by uid 89); 29 Oct 2018 23:26:49 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy= X-HELO: smtp3-g21.free.fr From: "Albert ARIBAUD (3ADEV)" To: libc-alpha@sourceware.org Cc: "Albert ARIBAUD (3ADEV)" Subject: [PATCH v3] Ensure mktime sets errno on error (bug 23789) Date: Tue, 30 Oct 2018 00:26:38 +0100 Message-Id: <20181029232638.5865-1-albert.aribaud@3adev.fr> Posix mandates that mktime set errno to EOVERFLOW on error, but the glibc mktime wasn't doing it so far. Fix this and add a test to prevent regressions. The fix also fixes the same issue in timegm. The test was run through 'make check' on i686-linux-gnu, then the fix was added and 'make check' run again. * time/Makefile: Add bug-mktime4. * time/bug-mktime4.c: New file. * time/mktime.c (__mktime_internal): Set errno to EOVERFLOW on error. (mktime): Move call to __tzset inside conditional. --- History: - v3: - time/tst-mktime4.c: switch to support/test-driver. - time/mktime: remove useless errno handling and move call to __tzset insde conditional. - v2: - __mktime_internal: set errno to EOVERFLOW upon failure. - mktime: detect failure from __tzset and __mktime_internal by clearing errno before call and checking it after. Final errno is as follows: - errno set by __mktime_internal if there was one; - otherwise, 0 if __mktime_internal returned a non-failure -1; - otherwise, errno set by __tzset if there was one; - otherwise, value of errno on entry in mktime. - v1: - mktime: set errno to EOVERFLOW if __mktime_internal returns -1 Notes: - v1 erroneously took any return value of -1 as a sign of error, regardless to whether errno was 0 or not; v2 handles the case where __mktime_internal return -1 as a correct value. - an alternative design was considered where every function called, directly or indirectly, from mktime would have been made to return a failure status but not change errno (and wrappers created to provide these function's original behavior). The change was too extensive, and had a high risk of breaking some behavior. - timegm() automatically benefits from this change too, i.e., it now reports failures properly with errno=EOVERFLOW. - __tzset may set errno (e.g. to ENOENT) and then __mktime may overwrite this with errno=EOVERFLOW (when failing) or errno=0 (when return valid -1). However, that was already the case also before the patch. time/Makefile | 2 +- time/bug-mktime4.c | 27 +++++++++++++++++++++++++++ time/mktime.c | 16 +++++++++++----- 3 files changed, 39 insertions(+), 6 deletions(-) create mode 100644 time/bug-mktime4.c diff --git a/time/Makefile b/time/Makefile index ec3e39dcea..743bd99f18 100644 --- a/time/Makefile +++ b/time/Makefile @@ -43,7 +43,7 @@ tests := test_time clocktest tst-posixtz tst-strptime tst_wcsftime \ tst-getdate tst-mktime tst-mktime2 tst-ftime_l tst-strftime \ tst-mktime3 tst-strptime2 bug-asctime bug-asctime_r bug-mktime1 \ tst-strptime3 bug-getdate1 tst-strptime-whitespace tst-ftime \ - tst-tzname tst-y2039 + tst-tzname tst-y2039 bug-mktime4 include ../Rules diff --git a/time/bug-mktime4.c b/time/bug-mktime4.c new file mode 100644 index 0000000000..14d04c669b --- /dev/null +++ b/time/bug-mktime4.c @@ -0,0 +1,27 @@ +#include +#include +#include +#include +#include + +static int +do_test (void) +{ + struct tm tm = { .tm_year = INT_MIN, .tm_mon = INT_MIN, .tm_mday = INT_MIN, + .tm_hour = INT_MIN, .tm_min = INT_MIN, .tm_sec = INT_MIN }; + errno = 0; + time_t tt = mktime (&tm); + if (tt != -1) + { + printf ("mktime() should have returned -1, returned %ld\n", (long int) tt); + return 1; + } + if (errno != EOVERFLOW) + { + printf ("mktime() returned -1, errno should be %d (EOVERFLOW) but is %d (%s)\n", EOVERFLOW, errno, strerror(errno)); + return 1; + } + return 0; +} + +#include "support/test-driver.c" diff --git a/time/mktime.c b/time/mktime.c index 00f0dec6b4..2e0c467147 100644 --- a/time/mktime.c +++ b/time/mktime.c @@ -49,6 +49,7 @@ # define LEAP_SECONDS_POSSIBLE 1 #endif +#include #include #include @@ -435,7 +436,10 @@ __mktime_internal (struct tm *tp, useful than returning -1. */ goto offset_found; else if (--remaining_probes == 0) - return -1; + { + __set_errno (EOVERFLOW); + return -1; + } /* We have a match. Check whether tm.tm_isdst has the requested value, if any. */ @@ -507,7 +511,10 @@ __mktime_internal (struct tm *tp, if (INT_ADD_WRAPV (t, sec_adjustment, &t) || ! (mktime_min <= t && t <= mktime_max) || ! convert_time (convert, t, &tm)) - return -1; + { + __set_errno (EOVERFLOW); + return -1; + } } *tp = tm; @@ -522,13 +529,12 @@ __mktime_internal (struct tm *tp, time_t mktime (struct tm *tp) { +# if defined _LIBC || NEED_MKTIME_WORKING + static mktime_offset_t localtime_offset; /* POSIX.1 8.1.1 requires that whenever mktime() is called, the time zone names contained in the external variable 'tzname' shall be set as if the tzset() function had been called. */ __tzset (); - -# if defined _LIBC || NEED_MKTIME_WORKING - static mktime_offset_t localtime_offset; return __mktime_internal (tp, __localtime_r, &localtime_offset); # else # undef mktime