hey joe, On Fri, Jul 17, 2009 at 09:27:53AM +0100, Joe Orton wrote: > > * i'm not sure but it seems like there might be a few corresponding free()'s > > missing from the mallocs/strdups. > > Yeah, everything here is malloc'ed but never free'd. We could set it up > to be free'd in the date extension's global destructor, but, it would > require modifying php_date.c which I am reluctant to do. I'm not sure > it makes much difference; the memory will remain allocated for the > lifetime of the process in any case, right?
my concern was that there might be a leak in a longer-running apache
module serving multiple requests (as opposed to a cgi script). i don't
have any evidence that this is the case though.
> > * it looks like load_zone_table() is called unconditionally, even if
> > it's not needed (i.e. a scall to localtime() will trigger it)
>
> I think this is necessary, though I didn't realize that before v6, and
> didn't explain it at all in the v6 comments. I've added some better
> commentary now in v7 - the problem is that we need the fake tzdb->data
> array to be created once the tzdb is created, since
> timezone_identifiers_list() peeks directly into it.
assuming you don't want to patch php_date.c, you might be right. i think
it's unfortunate though, as some of the simpler date functions are used
quite frequently and this is adding extra overhead due to a corner case.
> > * is parsing the comments really necessary? i don't see any php functions
> > that use them.
>
> They are exposed by the getLocation() interface -
d'oh, missed that one.
> > also, before i came to the conclusion that some kind of intermediate
> > hashtable was needed to do the mapping (at which point i gave up and
> > figured i'd wait to hear back from you :), i had done a slightly
> > simpler implementation of the parsing using a clean mmap'd buffer
> > and sscanf with "%ms" type formats which avoids a lot of the hard-coded
> > lengths etc[1]. if you're interested i can hack that code into the
> > load_zone_info (and parse_iso6709) to make a leaner/cleaner/more efficient
> > implementation.
>
> Does it end up being much simpler without relying on sscanf/%ms? I
> would rather this code is portable across older glibcs (e.g. so it works
> on older versions of RHEL).
if you keep the assumptions in the existing patch wrt max length of certain
fields you could do it with static arrays (ex: %63s) or even one of those
struct objects directly. i'll throw something your way if this saturday
continues being as boring as it is :)
also, something else i noticed:
strncpy(i->name, name, sizeof i->name);
won't null-terminate i->name in the case that strlen(name) >= 64. Then again,
i can't imagine the actual situation where a timezone name would actually
be that long (or where the admin would have a system that people could
inject malicious tzdata files)... so i guess it's more of a principle
thing that i have to point it out :)
sean
-
signature.asc
Description: Digital signature
