Date: Tue, 26 Nov 2013 18:32:12 -0500 From: Rich Felker <dalias@...ifal.cx> To: musl@...ts.openwall.com Subject: Re: [PATCH] Add support for leap seconds in zoneinfo files 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
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.