Re: [tz] localtime crash and fix

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@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.

On 2023-11-25 00:30, Alois Treindl via tz wrote:
The size of local_starage is in the range of 80 kb, not exactly something for a "small stack".
True. It'd be nice to make it smaller, and to allocate more RAM only if needed (as this would support even-larger TZif files), but nobody has had the time.
I think you save only 1kb extra spece for fullname, and some 30 bytes for tzhead.
It's more that we might as well not limit the TZ string to 1 KiB. That is, as long as we have allocated that 80 KiB object anyway, we might as well use it to support TZ strings longer than 1 KiB, since it's easy. I installed the attached commentary to try to make this a bit clearer.
participants (2)
-
Alois Treindl
-
Paul Eggert