From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.9 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_EF,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 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 dcvr.yhbt.net (Postfix) with ESMTPS id 4376A1F461 for ; Tue, 3 Sep 2019 19:56:40 +0000 (UTC) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:to:cc:references:from:subject:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; q=dns; s=default; b=ir57fYEnoEuJ22fS gjCKDsuD/s3V20XoazD6skaaHyTj1BV7b/wkg0wZ4mwWTH6Ex33zbjLIBgsHCR6v 2uZVw2J01VFuy8KqFSKFGB9ZxURY2srbwlSQXYofq5TmDs3BQlwTC14uo06YcjCo 20Q4vh0pmZmYKTmYP4I4hcbn0Kk= 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:to:cc:references:from:subject:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; s=default; bh=mBr+JY1fQLFuJDx5VUTtNJ N3HKg=; b=GXzXgn8X/AJinqX1XOtVOLBvz7K701XeKdoBndpZQdzy8g20C2KIyO C9BRwbFZWWi4DKYwyL8IapTpn60k4+zKE51cbkXi0dVwSNCCXVC0/4/3OFWZLWG7 /OIJDmXut5nxg5DMaZIJlDaWaarM4PSFesIqzZRuUGz1yHnLTCz7g= Received: (qmail 125545 invoked by alias); 3 Sep 2019 19:56:37 -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 Received: (qmail 125261 invoked by uid 89); 3 Sep 2019 19:56:35 -0000 Authentication-Results: sourceware.org; auth=none X-HELO: mail-qt1-f194.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=to:cc:references:from:openpgp:autocrypt:subject:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=MimykjuzcfMxffDAp9QTbQQKbKK0VN4KLKUZFGrCQs4=; b=sqpZDDDva+oRQD8HedkoLVCMAg30bjhdp5EJN8Ah0VT+fMgOAgEmkuQBClS3oNMybe qKY42WgBPSb8rh+jho0FEqSgI+U66ViXaTWv7rou2O66F1v35OZfTHE1mPaTvGGI3URC xIjWsj/XmhA1zVNtZ0JugEzUbW36hstEkTNvzl7TVCMweokQCDuHMz+JN7UIzZdg/6PG d1k3dK5fd6t+N0MXrmYMZpVQHKTRAm1p9rL0dKqiDo5tpsUxjCOZEsNrM0ouoRXoLzDE S18U069jrjeqBr0LIB8/g+pzjSFCoG9LIOTyxaFPqJJXsU2loCpZn7ZBJym8p1vm3uAv MF3g== To: Zack Weinberg , libc-alpha@sourceware.org Cc: Joseph Myers , Florian Weimer , Lukasz Majewski , Alistair Francis , Stepan Golosunov , Arnd Bergmann , Samuel Thibault References: <20190828153236.18229-1-zackw@panix.com> <20190828153236.18229-10-zackw@panix.com> From: Adhemerval Zanella Openpgp: preference=signencrypt Subject: Re: [PATCH v2 09/10] Warn when gettimeofday is called with non-null tzp argument. Message-ID: Date: Tue, 3 Sep 2019 16:56:26 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20190828153236.18229-10-zackw@panix.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit On 28/08/2019 12:32, Zack Weinberg wrote: > Since there are no known uses of gettimeofday's vestigial "get time > zone" feature that are not bugs, add a fortify-style wrapper inline to > sys/time.h that issues a warning whenever gettimeofday is called with > a second argument that is not a compile-time null pointer > constant. > > At present this is only possible with GCC; clang does not implement > attribute((warning)). The wrapper is only activated when __OPTIMIZE__ > is defined because it throws false positives when optimization is off, > even though it's an always-inline function. > > An oversight in the implementation of __builtin_constant_p causes it > to fail to detect compile-time *pointer* constants unless they are > cast to an integer of a different size. (Loss of data in this cast is > harmless; the overall expression is still constant if and only if the > original pointer was.) This is GCC bug 95514. Thanks to > Kamil Cukrowski for the workaround. > As a precaution, I added a static assertion to debug/warning-nop.c to > make sure that the cast _is_ casting to an integer of a different > size; this is too unlikely a scenario to be worth checking in the > public header, but if someone ever adds a port where short is the > same size as intptr_t, we'll still catch it. The conditionals of making this work without false-positive seems quite specific and fragile (gcc-only, optimized build) and the cast hack make even more suspicious it won't break in some arcane build environment (since it is an exported header, not internally used). I would prefer to just wait gcc to correctly fix it and set its minimum version to enable it with the expected semantic. > > Also make the public prototype of gettimeofday declare its second > argument with type "void *" unconditionally, consistent with POSIX. This is ok though. > > * time/sys/time.h (__timezone_ptr_t): Delete. > (gettimeofday): Always declare second argument with type "void *". > When possible, wrap with a fortify-style inline function that > detects non-null or non-constant second argument and issues a > warning. Improve commentary. > (settimeofday): Improve commentary. > > * time/gettimeofday.c (gettimeofday): Declare second argument with > type "void *". > * debug/warning-nop.c: Include sys/time.h and stdint.h. > Add static_assert to verify the requirements of the workaround > for GCC bug 95514. > --- > debug/warning-nop.c | 9 +++++++++ > time/gettimeofday.c | 4 ++-- > time/sys/time.h | 47 ++++++++++++++++++++++++++++++++++----------- > 3 files changed, 47 insertions(+), 13 deletions(-) > > diff --git a/debug/warning-nop.c b/debug/warning-nop.c > index 8eeea396c3..3eab53b78f 100644 > --- a/debug/warning-nop.c > +++ b/debug/warning-nop.c > @@ -67,4 +67,13 @@ nop (void) > #define __builtin___strncpy_chk(dest, src, len, bos) NULL > #define __builtin_object_size(bos, level) 0 > > +/* The code in sys/time.h that uses __warndecl has to work around GCC > + bug 91554. The work-around is only effective if intptr_t is not > + the same size as short. */ > +#include > +_Static_assert (sizeof (intptr_t) != sizeof (short), > + "workaround for GCC bug 91554 in sys/time.h" > + " is only effective when short is smaller than a pointer"); > + > #include > +#include > diff --git a/time/gettimeofday.c b/time/gettimeofday.c > index c4f642631f..5bc91fc214 100644 > --- a/time/gettimeofday.c > +++ b/time/gettimeofday.c > @@ -23,10 +23,10 @@ > If *TZ is not NULL, clear it. > Returns 0 on success, -1 on errors. */ > int > -___gettimeofday (struct timeval *tv, struct timezone *tz) > +___gettimeofday (struct timeval *restrict tv, void *restrict tz) > { > if (__glibc_unlikely (tz != 0)) > - memset (tz, 0, sizeof *tz); > + memset (tz, 0, sizeof (struct timezone)); > > struct timespec ts; > if (__clock_gettime (CLOCK_REALTIME, &ts)) > diff --git a/time/sys/time.h b/time/sys/time.h > index 5dbc7fc627..a4e7fd20d1 100644 > --- a/time/sys/time.h > +++ b/time/sys/time.h > @@ -54,23 +54,48 @@ struct timezone > int tz_minuteswest; /* Minutes west of GMT. */ > int tz_dsttime; /* Nonzero if DST is ever in effect. */ > }; > - > -typedef struct timezone *__restrict __timezone_ptr_t; > -#else > -typedef void *__restrict __timezone_ptr_t; > #endif > > -/* Get the current time of day and timezone information, > - putting it into *TV and *TZ. If TZ is NULL, *TZ is not filled. > - Returns 0 on success, -1 on errors. > - NOTE: This form of timezone information is obsolete. > - Use the functions and variables declared in instead. */ > +/* Get the current time of day, putting it into *TV. > + If TZ is not null, *TZ must be a struct timezone, and both fields > + will be set to zero. > + Calling this function with a non-null TZ is obsolete; > + use localtime etc. instead. > + This function itself is semi-obsolete; > + most callers should use time or clock_gettime instead. */ > extern int gettimeofday (struct timeval *__restrict __tv, > - __timezone_ptr_t __tz) __THROW __nonnull ((1)); > + void *__restrict __tz) __THROW __nonnull ((1)); > + > +#if __GNUC_PREREQ (4,3) && defined __REDIRECT && defined __OPTIMIZE__ > +/* Issue a warning for use of gettimeofday with a non-null __tz argument. */ > +__warndecl (__warn_gettimeofday_nonnull_timezone, > + "gettimeofday with non-null or non-constant timezone parameter;" > + " this is obsolete and inaccurate, use localtime instead"); > + > +extern int __REDIRECT_NTH (__gettimeofday_alias, > + (struct timeval *__restrict __tv, > + void *__restrict __tz), gettimeofday) > + __nonnull ((1)); > + > +/* The double cast below works around a limitation in __builtin_constant_p > + in all released versions of GCC (as of August 2019). > + See . */ > +__fortify_function int > +__NTH (gettimeofday (struct timeval *__restrict __tv, void *__restrict __tz)) > +{ > + if (! (__builtin_constant_p ((short) (__intptr_t) __tz) && __tz == 0)) > + __warn_gettimeofday_nonnull_timezone (); > + > + return __gettimeofday_alias (__tv, __tz); > +} > +#endif > > #ifdef __USE_MISC > /* Set the current time of day and timezone information. > - This call is restricted to the super-user. */ > + This call is restricted to the super-user. > + Setting the timezone in this way is obsolete, but we don't yet > + warn about it because it still has some uses for which there is > + no alternative. */ > extern int settimeofday (const struct timeval *__tv, > const struct timezone *__tz) > __THROW; >