[php-maint] Bug#535770: Bug#535770: PHP system timezone patch for 5.3?

[sean finney]

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
- 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.alioth.debian.org/pipermail/pkg-php-maint/attachments/20090718/10e3d418/attachment-0003.pgp>