Minor nits. christos --- localtime.c.orig 2014-10-07 16:20:32.000000000 -0400 +++ localtime.c 2014-10-07 16:22:47.000000000 -0400 @@ -306,3 +306,3 @@ register int stored; - register int nread; + register ssize_t nread; typedef union { @@ -934,3 +934,2 @@ register bool load_ok; - static struct ttinfo zttinfo; @@ -1006,3 +1005,3 @@ */ - sp->ttis[0] = sp->ttis[1] = zttinfo; + memset(sp->ttis, 0, sizeof(sp->ttis)); sp->ttis[0].tt_gmtoff = -dstoffset; @@ -1131,3 +1130,3 @@ */ - sp->ttis[0] = sp->ttis[1] = zttinfo; + memset(sp->ttis, 0, sizeof(sp->ttis)); sp->ttis[0].tt_gmtoff = -stdoffset; @@ -1145,3 +1144,3 @@ sp->timecnt = 0; - sp->ttis[0] = zttinfo; + memset(sp->ttis, 0, sizeof(sp->ttis)); sp->ttis[0].tt_gmtoff = -stdoffset; @@ -2108,3 +2107,2 @@ - sp = lclptr; i = sp->leapcnt;
Back in the 1980s (when this code had its origins), NULL pointers weren't guaranteed to be all zeroes; having a static, otherwise unused structure (which was guaranteed to be initialized correctly) for initialization purposes was cheap portability insurance. (The insurance was taken out consistently, both for uniformity and to avoid problems if a pointer was added to a structure later.) Given updates to the C standard and waning interest in supporting old systems, this insurance may well no longer be needed. Meanwhile, changing... memset(sp->ttis, 0, sizeof(sp->ttis)); ...to... memset(sp->ttis, 0, sizeof sp->ttis); ...saves a character and is a reminder (to me, at least) that sizeof isn't a function. @dashdashado On Tue, Oct 7, 2014 at 4:25 PM, Christos Zoulas <christos@zoulas.com> wrote:
Minor nits.
christos
--- localtime.c.orig 2014-10-07 16:20:32.000000000 -0400 +++ localtime.c 2014-10-07 16:22:47.000000000 -0400 @@ -306,3 +306,3 @@ register int stored; - register int nread; + register ssize_t nread; typedef union { @@ -934,3 +934,2 @@ register bool load_ok; - static struct ttinfo zttinfo;
@@ -1006,3 +1005,3 @@ */ - sp->ttis[0] = sp->ttis[1] = zttinfo; + memset(sp->ttis, 0, sizeof(sp->ttis)); sp->ttis[0].tt_gmtoff = -dstoffset; @@ -1131,3 +1130,3 @@ */ - sp->ttis[0] = sp->ttis[1] = zttinfo; + memset(sp->ttis, 0, sizeof(sp->ttis)); sp->ttis[0].tt_gmtoff = -stdoffset; @@ -1145,3 +1144,3 @@ sp->timecnt = 0; - sp->ttis[0] = zttinfo; + memset(sp->ttis, 0, sizeof(sp->ttis)); sp->ttis[0].tt_gmtoff = -stdoffset; @@ -2108,3 +2107,2 @@
- sp = lclptr; i = sp->leapcnt;
On Tue, Oct 7, 2014, at 16:48, Arthur David Olson wrote:
Back in the 1980s (when this code had its origins), NULL pointers weren't guaranteed to be all zeroes; having a static, otherwise unused structure (which was guaranteed to be initialized correctly) for initialization purposes was cheap portability insurance. (The insurance was taken out consistently, both for uniformity and to avoid problems if a pointer was added to a structure later.) Given updates to the C standard and waning interest in supporting old systems, this insurance may well no longer be needed.
Just to be clear, the updates to the C standard regarding this issue consist of allowing one to initialize a non-static structure with the literal "{0}", rather than anything about being able to memset pointers or floats.
On Oct 7, 4:48pm, arthurdavidolson@gmail.com (Arthur David Olson) wrote: -- Subject: Re: [tz] localtime.c patch | Back in the 1980s (when this code had its origins), NULL pointers weren't | guaranteed to be all zeroes; having a static, otherwise unused structure | (which was guaranteed to be initialized correctly) for initialization | purposes was cheap portability insurance. (The insurance was taken out | consistently, both for uniformity and to avoid problems if a pointer was | added to a structure later.) Given updates to the C standard and waning | interest in supporting old systems, this insurance may well no longer be | needed. I don't think that there are any systems where this code will compile that have this limitation. Strictly speaking you are right of course. | Meanwhile, changing... | memset(sp->ttis, 0, sizeof(sp->ttis)); | ...to... | memset(sp->ttis, 0, sizeof sp->ttis); | ...saves a character and is a reminder (to me, at least) that sizeof isn't | a function. There is a bug fix there too; in one of the cases sp->ttis[1] is not initialized, and returns trash to the user. christos
Thanks for reviewing the code; this found a real bug and prompted me to find another. Plus, you found some other things worth sprucing up. To my mind the most important fix in your patch is removing that stray initialization in leapcorr; this fixes a bug in the implementation of the new (2014g) functions time2posix_z and posix2time_z. Christos Zoulas wrote:
There is a bug fix there too; in one of the cases sp->ttis[1] is not initialized, and returns trash to the user.
Sorry, I don't see the bug there; can you explain? I assume you're talking about either the code labeled "only standard time" or the code labeled "so, we're off a little", and in both cases there is only one time type, so the rest of the code should never access sp->ttis[1] and there should be no need to initialize it. Although C99-and-later guarantees that memset(..., 0, ...) sets the struct ttinfo members (which are int, bool, and int_fast32_t) to zero, tzcode assumes only C89 which as I understand it does not provide that guarantee, so to be safe we should avoid memset here. It's easy enough to initialize the remaining structure members by hand. The change to use ssize_t won't fix bugs on modern POSIX platforms, since POSIX nowadays requires int to be at least 32 bits, but the change is probably worth doing for clarity anway, and also to port to ancient POSIX which allowed 16-bit int. However, POSIX weirdly says that ssize_t might not be able to store values less than -1, which means subtracting a value from ssize_t is a no-no if the result might be less than -1, which means we need to redo a subtraction that might compute such a result. Furthermore, the loop from 0 to nread would need an index type other than 'int', but there it's clearer just to use memmove (something that C89 does guarantee). In trying out the code under 'valgrind' I found that the command "zdump ''" tickled some other uninitialized-storage usages: the "so, we're off a little" code doesn't initialize charcnt, goback, goahead, or defaulttype. I hope the attached proposed patch fixes all these problems; please let us know if it misses anything.
Paul Eggert said:
Although C99-and-later guarantees that memset(..., 0, ...) sets the struct ttinfo members (which are int, bool, and int_fast32_t) to zero, tzcode assumes only C89 which as I understand it does not provide that guarantee, so to be safe we should avoid memset here.
No. I can't track it down right now, but the view of WG14 was always that the representations of types stuff added in C99 was intended to have always applied to C90 as well. See Defect Report 069 for some stuff on this as well.
However, POSIX weirdly says that ssize_t might not be able to store values less than -1,
Huh? POSIX is built on C, and C doesn't have any type which is signed but with a minimum of greater than -127. I can see that POSIX says "The type ssize_t shall be capable of storing values at least in the range [-1, {SSIZE_MAX}].", but that doesn't exempt it from the C rules concerning types. If the maximum value of ssize_t is (say) 8191, then the minimum is either -8191 or -8192.
which means subtracting a value from ssize_t is a no-no if the result might be less than -1, which means we need to redo a subtraction that might compute such a result.
Not so. -- Clive D.W. Feather | If you lie to the compiler, Email: clive@davros.org | it will get its revenge. Web: http://www.davros.org | - Henry Spencer Mobile: +44 7973 377646
On 10/08/2014 12:00 AM, Clive D.W. Feather wrote:
the view of WG14 was always that the representations of types stuff added in C99 was intended to have always applied to C90
Thanks, that's good to know if we run into this issue again in the future.
I can see that POSIX says "The type ssize_t shall be capable of storing values at least in the range [-1, {SSIZE_MAX}].", but that doesn't exempt it from the C rules concerning types.
Well, in *theory* ssize_t could be an extended integer type using one's complement and SSIZE_MAX could be 1, no? As far as I can tell nothing in the C standard prohibits that, and that would mean one couldn't necessarily store -2 into an ssize_t variable. Of course this is purely a theoretical objection, but I've always been suspicious of that POSIX rule about -1 and ssize_t -- the rule must be there for a reason, though they don't explain the reason -- and so prefer to avoid ssize_t except to hold the result of 'read'-like syscalls that are defined to return ssize_t.
Paul Eggert said:
I can see that POSIX says "The type ssize_t shall be capable of storing values at least in the range [-1, {SSIZE_MAX}].", but that doesn't exempt it from the C rules concerning types.
Well, in *theory* ssize_t could be an extended integer type using one's complement and SSIZE_MAX could be 1, no?
Indeed. Note that SSIZE_MAX is a POSIX value and is not necessarily the largest value of ssize_t.
As far as I can tell nothing in the C standard prohibits that, and that would mean one couldn't necessarily store -2 into an ssize_t variable.
I was about to say that extended integer types have to at least have the range of signed or unsigned char, as appropriate, but I can't find any such rule. So it does seem that you could have such a type. Mutter.
Of course this is purely a theoretical objection, but I've always been suspicious of that POSIX rule about -1 and ssize_t -- the rule must be there for a reason, though they don't explain the reason
I'd always assumed that that notation showed the values that POSIX puts in variables of that type. -- Clive D.W. Feather | If you lie to the compiler, Email: clive@davros.org | it will get its revenge. Web: http://www.davros.org | - Henry Spencer Mobile: +44 7973 377646
On Oct 7, 10:11pm, eggert@cs.ucla.edu (Paul Eggert) wrote: -- Subject: Re: [tz] localtime.c patch | Sorry, I don't see the bug there; can you explain? I assume you're talking | about either the code labeled "only standard time" or the code labeled "so, | we're off a little", and in both cases there is only one time type, so the rest | of the code should never access sp->ttis[1] and there should be no need to | initialize it. Well, this was the case before we returned a pointer of that data to the user. I prefer not to return random bits from the heap to the user. | In trying out the code under 'valgrind' I found that the command "zdump ''" | tickled some other uninitialized-storage usages: the "so, we're off a little" | code doesn't initialize charcnt, goback, goahead, or defaulttype. Thanks, that is a good idea. | I hope the attached proposed patch fixes all these problems; please let us know I'll take a look, thanks! christos
On 10/08/2014 06:20 AM, Christos Zoulas wrote:
I prefer not to return random bits from the heap to the user.
On my platform a timezone_t struct has 25488 bytes, and clearing them all would take a bit of time. Since the user can look at the heap anyway, clearing the bits wouldn't add security.
There were other NetBSD changes that were not carried forward, and perhaps they should
Thanks, the attached proposed patch should address those issues. I've left the issue of documenting return and errno values for another day.
On Oct 8, 11:24am, eggert@cs.ucla.edu (Paul Eggert) wrote: -- Subject: Re: [tz] localtime.c patch | Thanks, the attached proposed patch should address those issues. I've | left the issue of documenting return and errno values for another day. Thanks, this looks good. christos
On Oct 7, 10:11pm, eggert@cs.ucla.edu (Paul Eggert) wrote: -- Subject: Re: [tz] localtime.c patch | I hope the attached proposed patch fixes all these problems; please let us | know if it misses anything. Yes, this looks good. There were other NetBSD changes that were not carried forward, and perhaps they should: - localtime and friends can return NULL. This is not mentioned in the man pages or handled properly. Specially annoying is that ctime() just calls asctime(localtime()) without checking the result of localtime and this can make programs coredump without giving them a chance to handle the error. - timesub() doesn't set errno on overflow so none of the API functions that go through this error path set errno when they fail. It would be nice if the error conditions of all the functions were documented in the man pages and the errors were formalized (i.e. have them always set errno consistently depending on the error condition). Thanks, christos
Arthur David Olson said:
Back in the 1980s (when this code had its origins), NULL pointers weren't guaranteed to be all zeroes; having a static, otherwise unused structure (which was guaranteed to be initialized correctly) for initialization purposes was cheap portability insurance. (The insurance was taken out consistently, both for uniformity and to avoid problems if a pointer was added to a structure later.) Given updates to the C standard and waning interest in supporting old systems, this insurance may well no longer be needed.
Disagree. The standard still doesn't say that you can memset to 0 to get a NULL pointer. Embedded systems in particular may well have odd setups that mean that a NULL pointer isn't a zero bit pattern (I currently develop on a system where a char is 16 bits and there's something important at address zero). -- Clive D.W. Feather | If you lie to the compiler, Email: clive@davros.org | it will get its revenge. Web: http://www.davros.org | - Henry Spencer Mobile: +44 7973 377646
participants (5)
-
Arthur David Olson -
christos@zoulas.com -
Clive D.W. Feather -
Paul Eggert -
random832@fastmail.us