[10/12] Warn when gettimeofday is called with non-null tzp argument.
Commit Message
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
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.
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
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.
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;
>
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
* 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
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
* 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
@@ -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))
@@ -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;