[PATCH] always reset DST rules during tzset

[PATCH] always reset DST rules during tzset

[Benjamin Peterson]

do_tzset() did't always reset the DST transition rules r0 and r1. That means the
rules from older TZ settings could leak into newer ones.

The following program demonstrates this bug. It should print out the same
timezone twice but doesn't due to the leaky state.

int main() {
	time_t t = 0;
	struct tm p;
	setenv("TZ", "STD-1DST", 1);
	localtime_r(&t, &p);
	printf("%s\n", p.tm_zone);
	setenv("TZ", "STD-1DST,M3.2.0,M11.1.0", 1);
	tzset();
	setenv("TZ", "STD-1DST", 1);
	localtime_r(&t, &p);
	printf("%s\n", p.tm_zone);
	return 0;
}
---
 src/time/__tz.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/time/__tz.c b/src/time/__tz.c
index 51e66514..3ccfdd5d 100644
--- a/src/time/__tz.c
+++ b/src/time/__tz.c
@@ -130,6 +130,9 @@ static void do_tzset()
 
 	if (old_tz && !strcmp(s, old_tz)) return;
 
+	memset(r0, 0, sizeof r0);
+	memset(r1, 0, sizeof r1);
+
 	if (zi) __munmap((void *)zi, map_size);
 
 	/* Cache the old value of TZ to check if it has changed. Avoid
@@ -194,7 +197,6 @@ static void do_tzset()
 			const unsigned char *p;
 			__tzname[0] = __tzname[1] = 0;
 			__daylight = __timezone = dst_off = 0;
-			for (i=0; i<5; i++) r0[i] = r1[i] = 0;
 			for (p=types; p<abbrevs; p+=6) {
 				if (!p[4] && !__tzname[0]) {
 					__tzname[0] = (char *)abbrevs + p[5];
-- 
2.17.1

Re: [PATCH] always reset DST rules during tzset

[Rich Felker]

On Fri, Sep 14, 2018 at 03:46:55PM -0700, Benjamin Peterson wrote:
> do_tzset() did't always reset the DST transition rules r0 and r1. That means the
> rules from older TZ settings could leak into newer ones.
> 
> The following program demonstrates this bug. It should print out the same
> timezone twice but doesn't due to the leaky state.
> 
> int main() {
> 	time_t t = 0;
> 	struct tm p;
> 	setenv("TZ", "STD-1DST", 1);
> 	localtime_r(&t, &p);
> 	printf("%s\n", p.tm_zone);
> 	setenv("TZ", "STD-1DST,M3.2.0,M11.1.0", 1);
> 	tzset();
> 	setenv("TZ", "STD-1DST", 1);
> 	localtime_r(&t, &p);
> 	printf("%s\n", p.tm_zone);
> 	return 0;
> }
> ---
>  src/time/__tz.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/time/__tz.c b/src/time/__tz.c
> index 51e66514..3ccfdd5d 100644
> --- a/src/time/__tz.c
> +++ b/src/time/__tz.c
> @@ -130,6 +130,9 @@ static void do_tzset()
>  
>  	if (old_tz && !strcmp(s, old_tz)) return;
>  
> +	memset(r0, 0, sizeof r0);
> +	memset(r1, 0, sizeof r1);
> +
>  	if (zi) __munmap((void *)zi, map_size);
>  
>  	/* Cache the old value of TZ to check if it has changed. Avoid
> @@ -194,7 +197,6 @@ static void do_tzset()
>  			const unsigned char *p;
>  			__tzname[0] = __tzname[1] = 0;
>  			__daylight = __timezone = dst_off = 0;
> -			for (i=0; i<5; i++) r0[i] = r1[i] = 0;
>  			for (p=types; p<abbrevs; p+=6) {
>  				if (!p[4] && !__tzname[0]) {
>  					__tzname[0] = (char *)abbrevs + p[5];
> -- 
> 2.17.1

This looks right, but is there a reason you swapped the for loop out
for memsets? Either should work, but I probably preferred the loop in
the original since, due to -ffreestanding which is necessary when
building a libc, the compiler can't inline the memset automatically
with a builtin. I might however use the new header wrapper framework
to make it so (dependent on __GNUC__) memcpy and memset get redirected
to the __builtin_* versions except in the source files that define
them, in which case this kind of thing wouldn't matter.

Rich

[PATCH v2] always reset DST rules during tzset

[Benjamin Peterson]

do_tzset() did't always reset the DST transition rules r0 and r1. That means the
rules from older TZ settings could leak into newer ones.

The following program demonstrates this bug. It should print out the same
timezone twice but doesn't due to the leaky state.

int main() {
	time_t t = 0;
	struct tm p;
	setenv("TZ", "STD-1DST", 1);
	localtime_r(&t, &p);
	printf("%s\n", p.tm_zone);
	setenv("TZ", "STD-1DST,M3.2.0,M11.1.0", 1);
	tzset();
	setenv("TZ", "STD-1DST", 1);
	localtime_r(&t, &p);
	printf("%s\n", p.tm_zone);
	return 0;
}
---
 src/time/__tz.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/time/__tz.c b/src/time/__tz.c
index 51e66514..185642e8 100644
--- a/src/time/__tz.c
+++ b/src/time/__tz.c
@@ -130,6 +130,8 @@ static void do_tzset()
 
 	if (old_tz && !strcmp(s, old_tz)) return;
 
+	for (i=0; i<5; i++) r0[i] = r1[i] = 0;
+
 	if (zi) __munmap((void *)zi, map_size);
 
 	/* Cache the old value of TZ to check if it has changed. Avoid
@@ -194,7 +196,6 @@ static void do_tzset()
 			const unsigned char *p;
 			__tzname[0] = __tzname[1] = 0;
 			__daylight = __timezone = dst_off = 0;
-			for (i=0; i<5; i++) r0[i] = r1[i] = 0;
 			for (p=types; p<abbrevs; p+=6) {
 				if (!p[4] && !__tzname[0]) {
 					__tzname[0] = (char *)abbrevs + p[5];
-- 
2.17.1

Re: [PATCH] always reset DST rules during tzset

[Benjamin Peterson]

> This looks right, but is there a reason you swapped the for loop out
> for memsets?

I thought it might generate better code due to store merging. But I definitely
forgot about -ffreestanding.

My latest patch switches back to the loop.