First, GCC warns about zdump.c's new signed-versus-unsigned comparisons "timeptr->tm_wday >= sizeof ...." and similarly for tm_mon. The comparisons aren't needed here, since the values are guaranteed to be in range, so the easiest fix is to remove them.
The packaged versions of localtime and gmtime always return in-range values for tm_wday and tm_mon. However, zdump.c might be compiled with a version of localtime or gmtime that does something evil, so defensive programming may be in order. Casts along the lines shown below should do the trick. --ado if (timeptr->tm_wday < 0 || timeptr->tm_wday >= (int) (sizeof wday_name / sizeof wday_name[0])) wn = "???"; else wn = wday_name[timeptr->tm_wday]; if (timeptr->tm_mon < 0 || timeptr->tm_mon >= (int) (sizeof mon_name / sizeof mon_name[0])) mn = "???"; else mn = mon_name[timeptr->tm_mon];
Olson, Arthur David (NIH/NCI) said:
However, zdump.c might be compiled with a version of localtime or gmtime that does something evil, so defensive programming may be in order.
True.
Casts along the lines shown below should do the trick.
--ado
if (timeptr->tm_wday < 0 || timeptr->tm_wday >= (int) (sizeof wday_name / sizeof wday_name[0]))
if ((unsigned) timeptr->tm_wday >= sizeof wday_name / sizeof wday_name[0]) makes it one comparison instead of two. -- Clive D.W. Feather | Work: <clive@demon.net> | Tel: +44 20 8495 6138 Internet Expert | Home: <clive@davros.org> | Fax: +44 870 051 9937 Demon Internet | WWW: http://www.davros.org | Mobile: +44 7973 377646 Thus plc | |
"Clive D.W. Feather" <clive@demon.net> writes:
if ((unsigned) timeptr->tm_wday >= sizeof wday_name / sizeof wday_name[0])
makes it one comparison instead of two.
Hold on a second. Isn't this optimization invalid on hosts with padding bits where UINT_MAX == INT_MAX? On such hosts, (unsigned)-INT_MAX is 1, which is in range, even though -INT_MAX isn't in range.
Olson, Arthur David (NIH/NCI) said:
zdump.c might be compiled with a version of localtime or gmtime that does something evil,
Well, in that case all bets are off: zdump can't possibly work on systems whose localtime or gmtime functions do arbitrarily evil things. They might set tm_year to a trap representation, for example, and in that case zdump can dump core despite the redundant checking. If you prefer the check, that's fine, I suppose it won't hurt on any real host (except for performance). But could you please add a comment about it? Something like this: /* ** This range test defends against the off-chance that the system ** localtime or gmtime is buggy and returns out-of-range values. ** We don't know of any such systems, but we're playing it safe anyway. */ if (timeptr->tm_wday < 0 || timeptr->tm_wday >= (int) (sizeof wday_name / sizeof wday_name[0]))
Paul Eggert said:
if ((unsigned) timeptr->tm_wday >= sizeof wday_name / sizeof wday_name[0])
makes it one comparison instead of two.
Hold on a second. Isn't this optimization invalid on hosts with padding bits where UINT_MAX == INT_MAX? On such hosts, (unsigned)-INT_MAX is 1, which is in range, even though -INT_MAX isn't in range.
Doesn't matter for defensive coding. Okay, a value of -INT_MAX will print as "Tue" rather than "???", but who cares? Indeed, forget the test, add "???" to the end of the array, and just do: wday_name [timeptr->tm_wday & 7] Similarly for the month, put 4 sets of ??? and go & 15. -- Clive D.W. Feather | Work: <clive@demon.net> | Tel: +44 20 8495 6138 Internet Expert | Home: <clive@davros.org> | Fax: +44 870 051 9937 Demon Internet | WWW: http://www.davros.org | Mobile: +44 7973 377646 Thus plc | |
"Clive D.W. Feather" <clive@demon.net> writes:
Doesn't matter for defensive coding. Okay, a value of -INT_MAX will print as "Tue" rather than "???", but who cares?
We're talking about code that attempts to defend against nonexistent system bugs of a class that one cannot defend against in general. So to some extent we've gone off the deep end already. As long as we're going to such extremes, we might as well have the code output an appropriate danger signal (in this case "???") whenever any underlying problem can easily be detected.
Paul Eggert said:
Doesn't matter for defensive coding. Okay, a value of -INT_MAX will print as "Tue" rather than "???", but who cares?
We're talking about code that attempts to defend against nonexistent system bugs of a class that one cannot defend against in general. So to some extent we've gone off the deep end already. As long as we're going to such extremes, we might as well have the code output an appropriate danger signal (in this case "???") whenever any underlying problem can easily be detected.
In which case, wouldn't judicious use of assert() be even better? Point taken, though. -- Clive D.W. Feather | Work: <clive@demon.net> | Tel: +44 20 8495 6138 Internet Expert | Home: <clive@davros.org> | Fax: +44 870 051 9937 Demon Internet | WWW: http://www.davros.org | Mobile: +44 7973 377646 Thus plc | |
participants (3)
-
Clive D.W. Feather -
Olson, Arthur David (NIH/NCI) -
Paul Eggert