Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:38857 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 78461 invoked from network); 8 Jul 2008 14:45:13 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 8 Jul 2008 14:45:13 -0000 Authentication-Results: pb1.pair.com smtp.mail=jorton@redhat.com; spf=pass; sender-id=pass Authentication-Results: pb1.pair.com header.from=jorton@redhat.com; sender-id=pass Received-SPF: pass (pb1.pair.com: domain redhat.com designates 66.187.233.31 as permitted sender) X-PHP-List-Original-Sender: jorton@redhat.com X-Host-Fingerprint: 66.187.233.31 mx1.redhat.com Linux 2.6 Received: from [66.187.233.31] ([66.187.233.31:41819] helo=mx1.redhat.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id 20/68-32540-97D73784 for ; Tue, 08 Jul 2008 10:45:13 -0400 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id m68EjAd1008342 for ; Tue, 8 Jul 2008 10:45:10 -0400 Received: from turnip.manyfish.co.uk (vpn-13-235.rdu.redhat.com [10.11.13.235]) by int-mx1.corp.redhat.com (8.13.1/8.13.1) with ESMTP id m68Ej9X4013105 for ; Tue, 8 Jul 2008 10:45:09 -0400 Received: from jorton by turnip.manyfish.co.uk with local (Exim 4.69) (envelope-from ) id 1KGEN7-00005a-Ft for internals@lists.php.net; Tue, 08 Jul 2008 15:40:49 +0100 Date: Tue, 8 Jul 2008 15:40:49 +0100 To: internals@lists.php.net Message-ID: <20080708144049.GA17307@redhat.com> Mail-Followup-To: internals@lists.php.net MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="azLHFNyN32YCQGCU" Content-Disposition: inline User-Agent: Mutt/1.5.18 (2008-05-17) Organization: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in UK and Wales under Company Registration No. 03798903 Directors: Michael Cunningham (USA), Brendan Lane (Ireland), Matt Parson (USA), Charlie Peters (USA) X-Scanned-By: MIMEDefang 2.58 on 172.16.52.254 Subject: [PATCH] ext/date tzinfo structure references From: jorton@redhat.com (Joe Orton) --azLHFNyN32YCQGCU Content-Type: text/plain; charset=utf-8 Content-Disposition: inline The result of calling getTimezone() on a Date object results in a DateTimeZone object with a reference to the dateobj->time->tz_info object which may get later destroyed. This can cause unexpected script behaviour or interpreter crashes, test case in attachment (1). When I fixed this the obvious way, per attachment 3, by adding a clone, I wondered where the cloned tzinfo structures would get destroyed and I can't see anywhere. After looking at this further - so far as I can tell, the duplication of the tzinfo structures throughout this code is not actually necessary; when the structures are created they are referenced from the global tzcache and will hence last "forever" anyway. The structures are not changed anywhere either, again AFAICT; though they aren't treated as const so maybe I'm missing something there. So simply copying pointers around would simplify the code, fix leaks and fix the bug as well. That's attachment (2). What do you think? Regards, Joe --azLHFNyN32YCQGCU Content-Type: text/plain; charset=utf-8 Content-Disposition: attachment; filename="php_tzinfo-test.diff" Index: ext/date/tests/015.phpt =================================================================== RCS file: ext/date/tests/015.phpt diff -N ext/date/tests/015.phpt --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ ext/date/tests/015.phpt 8 Jul 2008 14:35:51 -0000 @@ -0,0 +1,22 @@ +--TEST-- +timezone object reference handling +--INI-- +date.timezone=UTC +--FILE-- +getTimezone(); +var_dump($tzold->getName()); +$dto->setTimezone(new DateTimeZone('US/Eastern')); +var_dump($tzold->getName()); +var_dump($dto->getTimezone()->getName()); +unset($dto); +var_dump($tzold->getName()); +echo "Done\n"; +?> +--EXPECTF-- +unicode(3) "UTC" +unicode(3) "UTC" +unicode(10) "US/Eastern" +unicode(3) "UTC" +Done --azLHFNyN32YCQGCU Content-Type: text/plain; charset=utf-8 Content-Disposition: attachment; filename="php_tzinfo.diff" Index: ext/date/php_date.c =================================================================== RCS file: /repository/php-src/ext/date/php_date.c,v retrieving revision 1.189 diff -u -r1.189 php_date.c --- ext/date/php_date.c 7 Jun 2008 22:41:02 -0000 1.189 +++ ext/date/php_date.c 8 Jul 2008 14:21:35 -0000 @@ -1341,14 +1341,6 @@ timelib_update_ts(t, tzi); ts = timelib_date_to_int(t, &error2); - /* if tz_info is not a copy, avoid double free */ - if (now->tz_info != tzi && now->tz_info) { - timelib_tzinfo_dtor(now->tz_info); - } - if (t->tz_info != tzi) { - timelib_tzinfo_dtor(t->tz_info); - } - timelib_time_dtor(now); timelib_time_dtor(t); @@ -1779,7 +1771,7 @@ newdateobj->time->tz_abbr = strdup(it_time->tz_abbr); } if (it_time->tz_info) { - newdateobj->time->tz_info = timelib_tzinfo_clone(it_time->tz_info); + newdateobj->time->tz_info = it_time->tz_info; } *data = &iterator->current; @@ -1966,7 +1958,7 @@ new_obj->time->tz_abbr = strdup(old_obj->time->tz_abbr); } if (old_obj->time->tz_info) { - new_obj->time->tz_info = timelib_tzinfo_clone(old_obj->time->tz_info); + new_obj->time->tz_info = old_obj->time->tz_info; } return new_ov; @@ -2185,9 +2177,6 @@ php_date_obj *intern = (php_date_obj *)object; if (intern->time) { - if (intern->time->tz_info) { - timelib_tzinfo_dtor(intern->time->tz_info); - } timelib_time_dtor(intern->time); } @@ -2220,16 +2209,10 @@ php_period_obj *intern = (php_period_obj *)object; if (intern->start) { - if (intern->start->tz_info) { - timelib_tzinfo_dtor(intern->start->tz_info); - } timelib_time_dtor(intern->start); } if (intern->end) { - if (intern->end->tz_info) { - timelib_tzinfo_dtor(intern->end->tz_info); - } timelib_time_dtor(intern->end); } @@ -2268,14 +2251,11 @@ timelib_time *now; timelib_tzinfo *tzi; timelib_error_container *err = NULL; - int free_tzi = 0, type = TIMELIB_ZONETYPE_ID, new_dst; + int type = TIMELIB_ZONETYPE_ID, new_dst; char *new_abbr; timelib_sll new_offset; if (dateobj->time) { - if (dateobj->time->tz_info) { - timelib_tzinfo_dtor(dateobj->time->tz_info); - } timelib_time_dtor(dateobj->time); } if (format) { @@ -2303,8 +2283,7 @@ tzobj = (php_timezone_obj *) zend_object_store_get_object(timezone_object TSRMLS_CC); switch (tzobj->type) { case TIMELIB_ZONETYPE_ID: - tzi = timelib_tzinfo_clone(tzobj->tzi.tz); - free_tzi = 1; + tzi = tzobj->tzi.tz; break; case TIMELIB_ZONETYPE_OFFSET: new_offset = tzobj->tzi.utc_offset; @@ -2317,8 +2296,7 @@ } type = tzobj->type; } else if (dateobj->time->tz_info) { - tzi = timelib_tzinfo_clone(dateobj->time->tz_info); - free_tzi = 1; + tzi = dateobj->time->tz_info; } else { tzi = get_timezone_info(TSRMLS_C); } @@ -2345,12 +2323,6 @@ dateobj->time->have_relative = 0; - if (type == TIMELIB_ZONETYPE_ID && now->tz_info != tzi) { - timelib_tzinfo_dtor(now->tz_info); - } - if (free_tzi) { - timelib_tzinfo_dtor(tzi); - } timelib_time_dtor(now); return 1; @@ -2860,10 +2832,7 @@ if (tzobj->type != TIMELIB_ZONETYPE_ID) { php_error_docref(NULL TSRMLS_CC, E_WARNING, "Can only do this for zones with ID for now"); } - if (dateobj->time->tz_info) { - timelib_tzinfo_dtor(dateobj->time->tz_info); - } - timelib_set_timezone(dateobj->time, timelib_tzinfo_clone(tzobj->tzi.tz)); + timelib_set_timezone(dateobj->time, tzobj->tzi.tz); timelib_unixtime2local(dateobj->time, dateobj->time->sse); } /* }}} */ @@ -3613,7 +3582,7 @@ clone->tz_abbr = strdup(dateobj->time->tz_abbr); } if (dateobj->time->tz_info) { - clone->tz_info = timelib_tzinfo_clone(dateobj->time->tz_info); + clone->tz_info = dateobj->time->tz_info; } dpobj->start = clone; @@ -3629,7 +3598,7 @@ clone->tz_abbr = strdup(dateobj->time->tz_abbr); } if (dateobj->time->tz_info) { - clone->tz_info = timelib_tzinfo_clone(dateobj->time->tz_info); + clone->tz_info = dateobj->time->tz_info; } dpobj->end = clone; } Index: ext/date/tests/015.phpt =================================================================== RCS file: ext/date/tests/015.phpt diff -N ext/date/tests/015.phpt --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ ext/date/tests/015.phpt 8 Jul 2008 14:21:35 -0000 @@ -0,0 +1,19 @@ +--TEST-- +timezone object reference handling +--INI-- +date.timezone=UTC +--FILE-- +getTimezone(); +var_dump($tzold->getName()); +$dto->setTimezone(new DateTimeZone('US/Eastern')); +var_dump($tzold->getName()); +var_dump($dto->getTimezone()->getName()); +echo "Done\n"; +?> +--EXPECTF-- +unicode(3) "UTC" +unicode(3) "UTC" +unicode(10) "US/Eastern" +Done --azLHFNyN32YCQGCU Content-Type: text/plain; charset=utf-8 Content-Disposition: attachment; filename="php_tzinfo-2.diff" Index: php_date.c =================================================================== RCS file: /repository/php-src/ext/date/php_date.c,v retrieving revision 1.189 diff -u -r1.189 php_date.c --- php_date.c 7 Jun 2008 22:41:02 -0000 1.189 +++ php_date.c 8 Jul 2008 14:31:05 -0000 @@ -2824,7 +2824,7 @@ tzobj->type = dateobj->time->zone_type; switch (dateobj->time->zone_type) { case TIMELIB_ZONETYPE_ID: - tzobj->tzi.tz = dateobj->time->tz_info; + tzobj->tzi.tz = timelib_tzinfo_clone(dateobj->time->tz_info); break; case TIMELIB_ZONETYPE_OFFSET: tzobj->tzi.utc_offset = dateobj->time->z; --azLHFNyN32YCQGCU--