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

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. UTC
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(-)

Comments

Paul Eggert Aug. 20, 2019, 6:52 p.m. UTC | #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. UTC | #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. UTC | #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. UTC | #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. UTC | #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. UTC | #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. UTC | #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. UTC | #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 mbox series

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;