Re: [PATCH] Add support for leap seconds in zoneinfo files

[Rich Felker <dalias@aerifal.cx>]

On Tue, Nov 26, 2013 at 06:53:41PM +0000, Laurent Bercot wrote:
> 
>  Attached.
> 
>  I tried to make it as simple and musl-coding-style-compliant as possible.
>  One line adds support for TZif3 (it costs nothing once we have TZif2 and
> we can ignore the extensions).

That should be ok.

>  The other changes are ifdef-guarded and can be ignored with -DNO_LEAPSECONDS,
> for hardcore POSIX fans. ;) But posix/ rules don't have leap seconds anyway,
> only right/ ones do: if you don't like #ifdef forests, feel free to remove the
> guards.

That would probably be done for commit, but it's not important at this
time.

>  By the way, what's the musl policy on internal sanity checks ? In __tz.c,
> musl doesn't check anything after the magic numbers, so if the zoneinfo file
> is bad later on, musl can crash, or worse, silently return inconsistent or
> wrong results. I understand that it's impossible to escalate an error to the
> user when the entry point returns a void, but wouldn't it be safer, in this
> case as well as in similar cases, to abort the program ? I tend to think that
> even raising a SIGSEGV manually would be better than silent undefined
> behaviour.

The code is careful to restrict TZ to a system-wide timezone file if
running with elevated privileges. In other cases, providing a corrupt
or intentionally invalid file is user error, comparaible to providing
a corrupt or invalid .so file. I agree it would be "nice" to avoid
runaway incorrect behavior on invalid files, but I question whether
the cost is justifiable. Not only would it require a full zoneinfo
file validator; it would also require loading the file into memory,
rather than mmapping it, since with mmap the contents could (at least
in theory) change after validation is performed (sadly, Linux lacks
MAP_COPY).

By the way, this brings me to one part of the proposed patch that I'd
definitely like to see changed: the pre-parsing the leapseconds data
and storage in static memory. This both wastes static storage (800
bytes for 64-bit systems) and puts an arbitrary limit on the data size
where it's not needed. I'd really prefer that the leapsecond
processing happen direct from the mmapped space, just like the way
time zone transitions are handled. It would also be preferable to use
a binary search like what's used on the transition list instead of a
linear search; this change to binary search would probably more than
compensate for any performance loss from reading directly from the
mmapped data.

Other than that, I have not yet reviewed the patch in detail, but so
far it doesn't look too invasive.

>  I can also confirm that strftime() fails to null-terminate its output in some
> circumstances, most likely when the format string ends with a regular character
> instead of a conversion specifier. I'll submit a patch as soon as I have time to
> investigate more, unless you can fix it before I get to it.

I'll look into it and see if I can fix it first.

Rich