[10/12] Warn when gettimeofday is called with non-null tzp argument.

Message ID 20190820132152.24100-11-zackw@panix.com
State New
Headers show
Series
  • Y2038 preparation: use clock_[gs]ettime to implement the other time-getting and -setting functions
Related show

Commit Message

Zack Weinberg Aug. 20, 2019, 1:21 p.m.
At this stage I don’t think we can issue warnings for settimeofday
with a non-null tzp argument, nor for arbitrary use of struct
timezone.  But we can warn about gettimeofday with non-null tzp.

This uses a macro instead of an inline (fortify-style) function
because I got false positives with an inline, even with GCC 9.

	* time/sys/time.h (__timezone_ptr_t): Delete.
	(gettimeofday): Always declare second argument with type ‘void *’.
	When possible, wrap with a macro that detects non-null and
	non-constant second argument and issues a warning.
	Improve commentary.
	(settimeofday): Improve commentary.

	* time/gettimeofday.c (gettimeofday):
	Declare second argument as type ‘void *’.
---
 time/gettimeofday.c |  4 ++--
 time/sys/time.h     | 36 +++++++++++++++++++++++++-----------
 2 files changed, 27 insertions(+), 13 deletions(-)

-- 
2.23.0.rc1

Comments

Paul Eggert Aug. 20, 2019, 6:52 p.m. | #1
Zack Weinberg wrote:
> -    memset (tz, 0, sizeof *tz);

> +    memset (tz, 0, sizeof (struct timezone));


This change isn't necessary, and I prefer the previous version as it's easier to 
audit.
Zack Weinberg Aug. 20, 2019, 7:03 p.m. | #2
On Tue, Aug 20, 2019 at 2:52 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
>

> Zack Weinberg wrote:

> > -    memset (tz, 0, sizeof *tz);

> > +    memset (tz, 0, sizeof (struct timezone));

>

> This change isn't necessary, and I prefer the previous version as it's easier to

> audit.


I think you missed that this change also makes the type of `tz` be
`void *` (consistent with POSIX) so the older construct will not work
anymore.

zw
Paul Eggert Aug. 20, 2019, 7:06 p.m. | #3
Zack Weinberg wrote:
>>> -    memset (tz, 0, sizeof *tz);

>>> +    memset (tz, 0, sizeof (struct timezone));

>> This change isn't necessary, and I prefer the previous version as it's easier to

>> audit.

> I think you missed that this change also makes the type of `tz` be

> `void *` (consistent with POSIX) so the older construct will not work

> anymore.

> 

> zw


Ah, right you are. Sorry about the noise.
Adhemerval Zanella Aug. 21, 2019, 3:30 p.m. | #4
On 20/08/2019 10:21, Zack Weinberg wrote:
> At this stage I don’t think we can issue warnings for settimeofday

> with a non-null tzp argument, nor for arbitrary use of struct

> timezone.  But we can warn about gettimeofday with non-null tzp.

> 

> This uses a macro instead of an inline (fortify-style) function

> because I got false positives with an inline, even with GCC 9.


Would be troublesome to add a check for the new warning? Also is the
false positive a GCC defect or something limited with the inline 
usage?

> 

> 	* time/sys/time.h (__timezone_ptr_t): Delete.

> 	(gettimeofday): Always declare second argument with type ‘void *’.

> 	When possible, wrap with a macro that detects non-null and

> 	non-constant second argument and issues a warning.

> 	Improve commentary.

> 	(settimeofday): Improve commentary.

> 

> 	* time/gettimeofday.c (gettimeofday):

> 	Declare second argument as type ‘void *’.

> ---

>  time/gettimeofday.c |  4 ++--

>  time/sys/time.h     | 36 +++++++++++++++++++++++++-----------

>  2 files changed, 27 insertions(+), 13 deletions(-)

> 

> diff --git a/time/gettimeofday.c b/time/gettimeofday.c

> index 22a996a220..bd1fc3cb5e 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..1b6c112274 100644

> --- a/time/sys/time.h

> +++ b/time/sys/time.h

> @@ -54,23 +54,37 @@ 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 <time.h> 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)

> +/* Issue a warning for use of gettimeofday with a non-null __tz argument.  */

> +__warndecl (__warn_gettimeofday_timezone,

> +            "gettimeofday with non-null or non-constant timezone parameter;"

> +            " this is obsolete and inaccurate, use localtime instead");

> +

> +#define gettimeofday(__tv, __tz)                        \

> +  (((!__builtin_constant_p (__tz) || (__tz) != 0)       \

> +    ? __warn_gettimeofday_timezone ()                   \

> +    : (void) 0),                                        \

> +   (gettimeofday) (__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;

>
Zack Weinberg Aug. 21, 2019, 4:02 p.m. | #5
On Wed, Aug 21, 2019 at 11:30 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> On 20/08/2019 10:21, Zack Weinberg wrote:

> > At this stage I don’t think we can issue warnings for settimeofday

> > with a non-null tzp argument, nor for arbitrary use of struct

> > timezone.  But we can warn about gettimeofday with non-null tzp.

> >

> > This uses a macro instead of an inline (fortify-style) function

> > because I got false positives with an inline, even with GCC 9.

>

> Would be troublesome to add a check for the new warning?


Do we have any infrastructure for testing for warnings?  I don't know about any.

> Also is the

> false positive a GCC defect or something limited with the inline

> usage?


I didn't look into it in any detail, but the uses of __warndecl +
__builtin_constant_p in bits/string2.h inlines all follow the pattern

    __extern_always_inline T func (ptr)
    {
      if (__builtin_constant_p (ptr) && something_else_p (ptr))
__issue_warning ();
      ...
    }

that is, issue a warning if ptr is a compile-time constant with some
property.  What we want for gettimeofday is just the opposite,

    if (! (__builtin_constant_p (ptr) && ptr == NULL)) __issue_warning ();

that is, warn whenever ptr is _not_ a compile-time NULL.  I wouldn't
be surprised if GCC's earliest unreachable-code removal passes, the
ones that happen early enough to prevent __attribute__((warning))
diagnostics from triggering, were tuned precisely for what
bits/string2.h does, and don't handle this inverse case properly.

zw
Florian Weimer Aug. 21, 2019, 4:10 p.m. | #6
* Zack Weinberg:

> I didn't look into it in any detail, but the uses of __warndecl +

> __builtin_constant_p in bits/string2.h inlines all follow the pattern

>

>     __extern_always_inline T func (ptr)

>     {

>       if (__builtin_constant_p (ptr) && something_else_p (ptr))

> __issue_warning ();

>       ...

>     }

>

> that is, issue a warning if ptr is a compile-time constant with some

> property.  What we want for gettimeofday is just the opposite,

>

>     if (! (__builtin_constant_p (ptr) && ptr == NULL)) __issue_warning ();

>

> that is, warn whenever ptr is _not_ a compile-time NULL.  I wouldn't

> be surprised if GCC's earliest unreachable-code removal passes, the

> ones that happen early enough to prevent __attribute__((warning))

> diagnostics from triggering, were tuned precisely for what

> bits/string2.h does, and don't handle this inverse case properly.


I don't think this will work.  You would have to use something like
this:

  __builtin_constant_p (ptr != NULL) && ptr != NULL

Otherwise you will produce a warning every time someone uses the
gettimeofday wrapper in a function for which optimization has been
disabled.

Thanks,
Florian
Zack Weinberg Aug. 22, 2019, 1:07 p.m. | #7
On 8/21/19 12:10 PM, Florian Weimer wrote:
> * Zack Weinberg:

> 

>> I didn't look into it in any detail, but the uses of __warndecl +

>> __builtin_constant_p in bits/string2.h inlines all follow the pattern

>>

>>     __extern_always_inline T func (ptr)

>>     {

>>       if (__builtin_constant_p (ptr) && something_else_p (ptr))

>> __issue_warning ();

>>       ...

>>     }

>>

>> that is, issue a warning if ptr is a compile-time constant with some

>> property.  What we want for gettimeofday is just the opposite,

>>

>>     if (! (__builtin_constant_p (ptr) && ptr == NULL)) __issue_warning ();

>>

>> that is, warn whenever ptr is _not_ a compile-time NULL.  I wouldn't

>> be surprised if GCC's earliest unreachable-code removal passes, the

>> ones that happen early enough to prevent __attribute__((warning))

>> diagnostics from triggering, were tuned precisely for what

>> bits/string2.h does, and don't handle this inverse case properly.

> 

> I don't think this will work.  You would have to use something like

> this:

> 

>   __builtin_constant_p (ptr != NULL) && ptr != NULL

> 

> Otherwise you will produce a warning every time someone uses the

> gettimeofday wrapper in a function for which optimization has been

> disabled.


That is the behavior I was seeing when the wrapper was an extern inline,
but not when it is a macro.  I'm going to do more thorough testing.

Also, `__builtin_constant_p (ptr != NULL) && ptr != NULL` would warn
only for compile-time non-NULL, if I'm reading it right.  But in this
case we also want to issue a warning for any argument that isn't a
compile-time constant.  In other words, we want to warn for everything
_except_ compile-time NULL.  Hence `! (__builtin_constant_p (ptr) && ptr
== NULL)`.

zw
Florian Weimer Aug. 23, 2019, 1:05 p.m. | #8
* Zack Weinberg:

> On 8/21/19 12:10 PM, Florian Weimer wrote:

>> * Zack Weinberg:

>> 

>>> I didn't look into it in any detail, but the uses of __warndecl +

>>> __builtin_constant_p in bits/string2.h inlines all follow the pattern

>>>

>>>     __extern_always_inline T func (ptr)

>>>     {

>>>       if (__builtin_constant_p (ptr) && something_else_p (ptr))

>>> __issue_warning ();

>>>       ...

>>>     }

>>>

>>> that is, issue a warning if ptr is a compile-time constant with some

>>> property.  What we want for gettimeofday is just the opposite,

>>>

>>>     if (! (__builtin_constant_p (ptr) && ptr == NULL)) __issue_warning ();

>>>

>>> that is, warn whenever ptr is _not_ a compile-time NULL.  I wouldn't

>>> be surprised if GCC's earliest unreachable-code removal passes, the

>>> ones that happen early enough to prevent __attribute__((warning))

>>> diagnostics from triggering, were tuned precisely for what

>>> bits/string2.h does, and don't handle this inverse case properly.

>> 

>> I don't think this will work.  You would have to use something like

>> this:

>> 

>>   __builtin_constant_p (ptr != NULL) && ptr != NULL

>> 

>> Otherwise you will produce a warning every time someone uses the

>> gettimeofday wrapper in a function for which optimization has been

>> disabled.

>

> That is the behavior I was seeing when the wrapper was an extern inline,

> but not when it is a macro.  I'm going to do more thorough testing.


I don't think we should turn gettimeofday into a macro.  It's likely to
cause problems with some C++ wrapper libraries.

> Also, `__builtin_constant_p (ptr != NULL) && ptr != NULL` would warn

> only for compile-time non-NULL, if I'm reading it right.


Correct.

> But in this case we also want to issue a warning for any argument that

> isn't a compile-time constant.  In other words, we want to warn for

> everything _except_ compile-time NULL.  Hence `! (__builtin_constant_p

> (ptr) && ptr == NULL)`.


I think this is simply not possible because it will cause unpredictable
false positives.

Thanks,
Florian

Patch

diff --git a/time/gettimeofday.c b/time/gettimeofday.c
index 22a996a220..bd1fc3cb5e 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..1b6c112274 100644
--- a/time/sys/time.h
+++ b/time/sys/time.h
@@ -54,23 +54,37 @@  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 <time.h> 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)
+/* Issue a warning for use of gettimeofday with a non-null __tz argument.  */
+__warndecl (__warn_gettimeofday_timezone,
+            "gettimeofday with non-null or non-constant timezone parameter;"
+            " this is obsolete and inaccurate, use localtime instead");
+
+#define gettimeofday(__tv, __tz)                        \
+  (((!__builtin_constant_p (__tz) || (__tz) != 0)       \
+    ? __warn_gettimeofday_timezone ()                   \
+    : (void) 0),                                        \
+   (gettimeofday) (__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;