Re: [PATCH 1/6] parse-datetime, posixtm: avoid uninit access
On 24/11/17 00:31, Paul Eggert wrote: > Pádraig Brady wrote: >> It's not introducing any new issue I think, but >> seems to be triggering the compiler warning due >> to tm_year being explicitly set? > > Actually, I think it's pointing out an issue with the API. If the caller to > posixtime were to specify neither PDS_LEADING_YEAR nor PDS_TRAILING_YEAR, the > 'year' function would never get called, which means that tm0.tm_year would be > uninitialized. > > No caller ever does that. In practice, PDS_LEADING_YEAR and PDS_TRAILING_YEAR > are mutually exclusive and one or the other should always be specified. This > suggests that there should be just a single bit in the API, not two bits. I > installed the attached patch, which changes the API accordingly, in a way > that I > think will not break any current callers. Does it pacify your GCC? It does indeed since bitwise ops are not in all paths to initializing tm_year. thanks! Pádraig
Re: [PATCH 1/6] parse-datetime, posixtm: avoid uninit access
Pádraig Brady wrote:
It's not introducing any new issue I think, but
seems to be triggering the compiler warning due
to tm_year being explicitly set?
Actually, I think it's pointing out an issue with the API. If the caller to
posixtime were to specify neither PDS_LEADING_YEAR nor PDS_TRAILING_YEAR, the
'year' function would never get called, which means that tm0.tm_year would be
uninitialized.
No caller ever does that. In practice, PDS_LEADING_YEAR and PDS_TRAILING_YEAR
are mutually exclusive and one or the other should always be specified. This
suggests that there should be just a single bit in the API, not two bits. I
installed the attached patch, which changes the API accordingly, in a way that I
think will not break any current callers. Does it pacify your GCC?
From ce9dee4b2eb11d1a659d4ef595c4e699285882c0 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Fri, 24 Nov 2017 00:29:04 -0800
Subject: [PATCH] posixtm: remove PDS_LEADING_YEAR
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This changes the API slightly, in a hopefully-innocuous way.
Without this change the code had undefined behavior when a
caller specified neither PDS_LEADING_YEAR nor PDS_TRAILING_YEAR.
Problem reported by Pádraig Brady in:
https://lists.gnu.org/r/bug-gnulib/2017-11/msg00048.html
* NEWS: Mention this.
* lib/posixtm.c (posix_time_parse): Treat the absence of
PDS_TRAILING_YEAR as if PDS_LEADING_YEAR were present.
* lib/posixtm.h (PDS_LEADING_YEAR): Remove (actually, leave it
present, but define it as zero, for compatibility with existing
source code). All other PDS_* values moved up.
* tests/test-posixtm.c (LY): New macro.
(T): Use it. Do not expect a particular numeric encoding
for PDS_CENTURY etc.
---
ChangeLog| 18 +++
NEWS | 6
lib/posixtm.c| 4 +--
lib/posixtm.h| 13 +---
tests/test-posixtm.c | 90 +++-
5 files changed, 80 insertions(+), 51 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 700ee09..5152abb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,21 @@
+2017-11-24 Paul Eggert
+
+ posixtm: remove PDS_LEADING_YEAR
+ This changes the API slightly, in a hopefully-innocuous way.
+ Without this change the code had undefined behavior when a
+ caller specified neither PDS_LEADING_YEAR nor PDS_TRAILING_YEAR.
+ Problem reported by Pádraig Brady in:
+ https://lists.gnu.org/r/bug-gnulib/2017-11/msg00048.html
+ * NEWS: Mention this.
+ * lib/posixtm.c (posix_time_parse): Treat the absence of
+ PDS_TRAILING_YEAR as if PDS_LEADING_YEAR were present.
+ * lib/posixtm.h (PDS_LEADING_YEAR): Remove (actually, leave it
+ present, but define it as zero, for compatibility with existing
+ source code). All other PDS_* values moved up.
+ * tests/test-posixtm.c (LY): New macro.
+ (T): Use it. Do not expect a particular numeric encoding
+ for PDS_CENTURY etc.
+
2017-11-23 Paul Eggert
stat: work around Solaris bug with tv_nsec < 0
diff --git a/NEWS b/NEWS
index d3a300a..9ec3a15 100644
--- a/NEWS
+++ b/NEWS
@@ -42,6 +42,12 @@ User visible incompatible changes
DateModules Changes
+2017-11-24 posixtm Previously, callers had to specify either
+PDS_LEADING_YEAR or PDS_TRAILING_YEAR (but
+not both). Now, callers should specify
+only PDS_TRAILING_YEAR; leading years are
+requested by not specifying PDS_TRAILING_YEAR.
+
2017-08-14 fcntl-h This module now defaults O_CLOEXEC to a nonzero
value instead of to 0, as the 'open' and
'openat' modules now emulate O_CLOEXEC.
diff --git a/lib/posixtm.c b/lib/posixtm.c
index 030f704..e718664 100644
--- a/lib/posixtm.c
+++ b/lib/posixtm.c
@@ -45,7 +45,7 @@
touch -t [[CC]YY]mmddhhmm[.ss] FILE...
8, 10, or 12 digits, followed by optional .ss
-(PDS_LEADING_YEAR | PDS_CENTURY | PDS_SECONDS)
+(PDS_CENTURY | PDS_SECONDS)
touch mmddhhmm[YY] FILE... (obsoleted by POSIX 1003.1-2001)
8 or 10 digits, YY (if present) must be in the range 69-99
@@ -136,7 +136,7 @@ posix_time_parse (struct tm *tm, const char *s, unsigned
int syntax_bits)
pair[i] = 10 * (s[2*i] - '0') + s[2*i + 1] - '0';
p = pair;
- if (syntax_bits & PDS_LEADING_YEAR)
+ if (! (syntax_bits & PDS_TRAILING_YEAR))
{
if (! year (tm, p, len - 4, syntax_bits))
return false;
diff --git a/lib/posixtm.h b/lib/posixtm.h
index 090c15b..7674227 100644
--- a/lib/posixtm.h
+++ b/lib/posixtm.h
@@ -25,11 +25,14 @@
# include
/* POSIX Date Syntax flags. */
-# define PDS_LEADING_YEAR 1
-# define PDS_TRAILING_YEAR 2
-# define PDS_CENTURY 4
-# define PDS_SECONDS 8
-# define PDS_PRE_2000 16
+# define PDS_TRAILING_YEAR 1
+# define
Re: [PATCH 1/6] parse-datetime, posixtm: avoid uninit access
On 25/09/17 18:29, Paul Eggert wrote:
> * lib/parse-datetime.y (parse_datetime2):
> * lib/posixtm.c (posixtime):
> Do not access uninitialized storage, even though the resulting
> value is never used.
> ---
> ChangeLog| 8
> lib/parse-datetime.y | 16 ++--
> lib/posixtm.c| 7 ++-
> 3 files changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 386986ee7..9c6d73f72 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,11 @@
> +2017-09-25 Paul Eggert
> +
> + parse-datetime, posixtm: avoid uninit access
> + * lib/parse-datetime.y (parse_datetime2):
> + * lib/posixtm.c (posixtime):
> + Do not access uninitialized storage, even though the resulting
> + value is never used.
> +
> 2017-09-25 Bruno Haible
>
> vma-iter: Improvements for BSD platforms.
> diff --git a/lib/parse-datetime.y b/lib/parse-datetime.y
> index 9eff2dc3b..f8da02d3f 100644
> --- a/lib/parse-datetime.y
> +++ b/lib/parse-datetime.y
> @@ -2034,7 +2034,13 @@ parse_datetime2 (struct timespec *result, char const
> *p,
>if (pc.local_zones_seen)
> tm.tm_isdst = pc.local_isdst;
>
> - tm0 = tm;
> + tm0.tm_sec = tm.tm_sec;
> + tm0.tm_min = tm.tm_min;
> + tm0.tm_hour = tm.tm_hour;
> + tm0.tm_mday = tm.tm_mday;
> + tm0.tm_mon = tm.tm_mon;
> + tm0.tm_year = tm.tm_year;
> + tm0.tm_isdst = tm.tm_isdst;
>
>Start = mktime_z (tz, &tm);
>
> @@ -2064,7 +2070,13 @@ parse_datetime2 (struct timespec *result, char const
> *p,
> dbg_printf (_("error: tzalloc (\"%s\") failed\n"),
> tz2buf);
>goto fail;
> }
> - tm = tm0;
> + tm.tm_sec = tm0.tm_sec;
> + tm.tm_min = tm0.tm_min;
> + tm.tm_hour = tm0.tm_hour;
> + tm.tm_mday = tm0.tm_mday;
> + tm.tm_mon = tm0.tm_mon;
> + tm.tm_year = tm0.tm_year;
> + tm.tm_isdst = tm0.tm_isdst;
>Start = mktime_z (tz2, &tm);
>repaired = mktime_ok (tz2, &tm0, &tm, Start);
>tzfree (tz2);
> diff --git a/lib/posixtm.c b/lib/posixtm.c
> index 26a35dd3f..030f704f0 100644
> --- a/lib/posixtm.c
> +++ b/lib/posixtm.c
> @@ -182,7 +182,12 @@ posixtime (time_t *p, const char *s, unsigned int
> syntax_bits)
>if (! posix_time_parse (&tm0, s, syntax_bits))
> return false;
>
> - tm1 = tm0;
> + tm1.tm_sec = tm0.tm_sec;
> + tm1.tm_min = tm0.tm_min;
> + tm1.tm_hour = tm0.tm_hour;
> + tm1.tm_mday = tm0.tm_mday;
> + tm1.tm_mon = tm0.tm_mon;
> + tm1.tm_year = tm0.tm_year;
>tm1.tm_isdst = -1;
>t = mktime (&tm1);
>
>
This triggers the following warning with gcc-6.3
lib/posixtm.c:214:20: error: '*((void *)&tm0+20)' may be used
uninitialized in this function [-Werror=maybe-uninitialized]
if ((tm0.tm_year ^ tm->tm_year)
~^~
It's not introducing any new issue I think, but
seems to be triggering the compiler warning due
to tm_year being explicitly set?
How about the attached to ensure tm_year is set?
cheers,
Pádraig
>From d1917dcdf83e4e754f2c5606a729faa6566e9939 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?=
Date: Thu, 23 Nov 2017 17:01:04 -0800
Subject: [PATCH] posixtm: avoid maybe-uninitialized warnings
* lib/posixtm.c (year): Ensure that tm_year is initialized.
Since commit 619700e1, gcc-6.3 at least fails to compile with:
lib/posixtm.c: In function 'posixtime':
lib/posixtm.c:214:20: error: '*((void *)&tm0+20)' may be used
uninitialized in this function [-Werror=maybe-uninitialized]
if ((tm0.tm_year ^ tm->tm_year)
~^~
---
ChangeLog | 11 +++
lib/posixtm.c | 7 ++-
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/ChangeLog b/ChangeLog
index 700ee09..a24d7c2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2017-11-23 Pádraig Brady
+
+ posixtm: avoid maybe-uninitialized warnings
+ * lib/posixtm.c (year): Ensure that tm_year is initialized.
+ Since commit 619700e1, gcc-6.3 at least fails to compile with:
+ lib/posixtm.c: In function 'posixtime':
+ lib/posixtm.c:214:20: error: '*((void *)&tm0+20)' may be used
+ uninitialized in this function [-Werror=maybe-uninitialized]
+ if ((tm0.tm_year ^ tm->tm_year)
+ ~^~
+
2017-11-23 Paul Eggert
stat: work around Solaris bug with tv_nsec < 0
diff --git a/lib/posixtm.c b/lib/posixtm.c
index 030f704..bc75487 100644
--- a/lib/posixtm.c
+++ b/lib/posixtm.c
@@ -22,6 +22,7 @@
#include "posixtm.h"
+#include
#include
#include
#include
@@ -96,7 +97,7 @@ year (struct tm *tm, const int *digit_pair, size_t n, unsigned int syntax_bits)
break;
default:
- abort ();
+ assert (!"invalid year length");
}
return true;
@@ -110,6 +111,10 @@ posix_time_parse (struct tm *tm, const cha
Re: [PATCH 1/6] parse-datetime, posixtm: avoid uninit access
Thien-Thi Nguyen wrote: Maybe add a preemptive comment, or factor the assignments into a macro (w/ comment)? It's not worth a macro. Feel free to add a comment.
Re: [PATCH 1/6] parse-datetime, posixtm: avoid uninit access
() Paul Eggert () Mon, 25 Sep 2017 18:29:08 -0700 Do not access uninitialized storage, even though the resulting value is never used. [...] - tm0 = tm; + tm0.tm_sec = tm.tm_sec; + tm0.tm_min = tm.tm_min; + tm0.tm_hour = tm.tm_hour; + tm0.tm_mday = tm.tm_mday; + tm0.tm_mon = tm.tm_mon; + tm0.tm_year = tm.tm_year; + tm0.tm_isdst = tm.tm_isdst; Start = mktime_z (tz, &tm); These changes look like a step backward in code readability, which could prompt a naive programmer to propose their reversion in the future (ping-pong problem). Maybe add a preemptive comment, or factor the assignments into a macro (w/ comment)? -- Thien-Thi Nguyen --- (defun responsep (query) (pcase (context query) (`(technical ,ml) (correctp ml)) ...)) 748E A0E8 1CB8 A748 9BFA --- 6CE4 6703 2224 4C80 7502 signature.asc Description: PGP signature
