[tz] localtime crash and fix
Alois Treindl
alois at astro.ch
Sat Nov 25 08:30:58 UTC 2023
Thanks for fixing.
I still find the code unnecessarily obscure due to the use of two unions stacked inside each other.
> /* Input buffer for data read from a compiled tz file. */
> union input_buffer {
> /* The first part of the buffer, interpreted as a header. */
> struct tzhead tzhead;
>
> /* The entire buffer. */
> char buf[2 * sizeof(struct tzhead) + 2 * sizeof(struct state)
> + 4 * TZ_MAX_TIMES];
> };
>
> /* TZDIR with a trailing '/' rather than a trailing '\0'. */
> static char const tzdirslash[sizeof TZDIR] = TZDIR "/";
>
> /* Local storage needed for 'tzloadbody'. */
> union local_storage {
> /* The results of analyzing the file's contents after it is opened. */
> struct file_analysis {
> /* The input buffer. */
> union input_buffer u;
>
> /* A temporary state used for parsing a TZ string in the file. */
> struct state st;
> } u;
>
> /* The file name to be opened. */
> char fullname[max(sizeof(struct file_analysis), sizeof tzdirslash + 1024)];
> };
The size of local_starage is in the range of 80 kb, not exactly something for a "small stack".
The element buf is rather large.
I think you save only 1kb extra spece for fullname, and some 30 bytes for tzhead.
I do not think that these very minor saving justify the obscurity introduced in the code.
It makes it hard to read and hard to debug.
Please consider that others want to understand tzcode also.
> Am 24.11.2023 um 23:14 schrieb Paul Eggert <eggert at cs.ucla.edu>:
>
> On 2023-11-24 08:22, Alois Treindl via tz wrote:
>> The use of union to save memory is dangerous, in my opinion. There is no need in today's machines to save a few kilobytes of RAM for a process.
>> Not even in embedded software for watches.
>
> When the memory is on the stack it still makes sense in some cases to save even a few kilobytes of RAM, as highly-threaded apps often have surprisingly small stacks. And anyway, the union has nothing to do with this particular bug.
More information about the tz
mailing list