# Debian PHP patches - Thread: https://externals.io/message/46751 - Original archive: https://news-web.php.net/php.internals/46751 - Started by johannes@php.net on 2010-01-16T15:07:04+00:00 - Messages: 19 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #46751 — johannes@php.net — 2010-01-16T15:07:04+00:00 URL: https://externals.io/message/46751#46751 Raphael, I went over your patches from http://patch-tracker.debian.org/package/php5/5.3.1-2 and did quick reviews (didn't apply / test them or anything ...) Here some comments: 004-ldap_fix.patch: Do you have a test for this? when does it happen that ldap_value is `NULL` happen? - If that's an issue we should add it to our tree... 013-force_getaddrinfo.patch: Why doesn't the check work on Debian out of the box? On my (not Debian) Linux boxes it works. anything needed for people compiling PHP on Debian themselves? 019-z_off_t_as_long.patch Certainly nothing for our tree 034-apache2_umask_fix.patch 036-fd_setsize_fix.patch Any reproduce case where this is an issue? 043-recode_size_t.patch Why is that needed? The name of the patch suggests that str_len is used in a place where an size_t is expected but then the location of this check is wrong. Additionally in case this check is tirggered the function will return without error message. 045-exif_nesting_level.patch How did you decide on the limit of 250? 047-zts_with_dl.patch I don't support this. `dl()` can be used by CLI users for loading Gtk or such on demand but avoiding to load (and inititalize) all the Gtk stuff every time they use PHP and avoid messing with config files... 052-phpinfo_no_configure.patch Neither do I support this. Users might benefit when building own modules or might be interested in the configure line for other reasons ... 053-extension_api.patch When adding random options to command line tools please mark them as Debian specific. Additionally the man page should be updated. 100-recode_is_shared.patch The conflict between MySQL and recode should only happen with an old libmysql (3.23?) not sure about imap ... but in your case the patch might make sense, while I won't directly apply it to our tree as usually people will build extensions to load them together... 101-sqlite_is_shared.patch Not sure if we do any modifications to SQLite but in general we prefer to use the bundled lib so users get the same behavior on all platforms. 108-64_bit_datetime.patch 116-posixness_fix.patch Why is that needed on our system? - I never saw an issue about these and if they are missing there should be build issues. use_embedded_timezonedb.patch Like with the SQLite note above we prefer to have the same behavior over all platforms by using a common time database. force_libmysqlclient_r.patch Why are you using the reentrant version of this library which is slower than the "regular" one? Did you consider mysqlnd? 009_ob-memory-leaks.patch Any test case? (While this looks good on first sight) exif_read_data-segfault.patch Any sample case for this? Any bug report? sybase-alias.patch I have no idea about MS SQL and Sybase but "randomly" aliassing looks bad to me. strcmp_null-OnUpdateErrorLog.patch You should at least ad an "echo 'done';" or such to the expect section to make sure it really works ... I skipped the patches related to build system and such and maybe skipped one or two by accident .... johannes --- ## #46754 — Raphael Geissert — 2010-01-16T18:10:21+00:00 URL: https://externals.io/message/46751#46754 · In reply to: #46751 Johannes, Johannes Schlüter wrote: > Raphael, > > I went over your patches from > http://patch-tracker.debian.org/package/php5/5.3.1-2 and did quick > reviews (didn't apply / test them or anything ...) Although I would have preferred you wait for me to submit each patch individually with enough information (because I've only been like two years co-maintaining the packages and most patches were added by others, and most before I joined), thanks. I'll try to comment on each but I don't know about them all and can't check them right now (I'm short on time). > > Here some comments: > > 004-ldap_fix.patch: > Do you have a test for this? when does it happen that ldap_value is > `NULL` happen? - If that's an issue we should add it to our tree... > I don't have a test, but here's the bug report that lead to that patch: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=205405 > 013-force_getaddrinfo.patch: > Why doesn't the check work on Debian out of the box? On my (not > Debian) Linux boxes it works. anything needed for people compiling PHP > on Debian themselves? A 2004 changelog entry says: - Add 013-force_getaddrinfo.patch, so that getaddrinfo support is always enabled (instead of doing check during build). So it looks it might be ok to disable it. > > 019-z_off_t_as_long.patch > Certainly nothing for our tree > I found these two entries: * Re-enable 019-z_off_t_as_long.patch, which is needed to make sure that LFS-enabled SAPIs can still use zlib file functions correctly. * Add 019-z_off_t_as_long.patch, including local headers for zlib, forcing off_t = long for gzip file functions, however disable it for now, as we'll only need it if we reenable LFS (closes: #208608) http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=208608 (N.B. at Debian we do enable LFS -- see below) > 034-apache2_umask_fix.patch > 036-fd_setsize_fix.patch > Any reproduce case where this is an issue? * Added several patches, yanked from the Fedora PHP sources: - 034-apache2_umask_fix.patch, fixes umask not being properly reset after each request (closes: #286225) - 036-fd_setsize_fix.patch, fixes misuse of FD_SET() http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=286225 > > 043-recode_size_t.patch > Why is that needed? The name of the patch suggests that str_len is > used in a place where an size_t is expected but then the location > of this check is wrong. Additionally in case this check is tirggered > the function will return without error message. * Add 043-recode_size_t.patch to fix 32/64-bit issues causing the recode extension to segfault on alpha/amd64/ia64 (closes: #294986) http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=294986 and later... * zend_parse_parameters does not handle size_t's, causing issues with 043-recode_size_t.patch and segmentation faults for recode-using pages. changed problematic parameters back to "int" and added an overflow check. thanks to Thomas Stegbauer, Tim Dijkstra, Bart Cortooms, Sebastian Göbel, and Vincent Tondellier for their reports. closes: #459020. http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=459020 > > 045-exif_nesting_level.patch > How did you decide on the limit of 250? * Add 045-exif_nesting_level.patch to bump the exif header parsing max nesting level to something that actually works with most JPEG images. > > 047-zts_with_dl.patch > I don't support this. `dl()` can be used by CLI users for loading Gtk or > such on demand but avoiding to load (and inititalize) all the Gtk > stuff every time they use PHP and avoid messing with config files... > > 052-phpinfo_no_configure.patch > Neither do I support this. Users might benefit when building own > modules or might be interested in the configure line for other > reasons ... I will later be commenting on these two. > > 053-extension_api.patch > When adding random options to command line tools please mark them as > Debian specific. Additionally the man page should be updated. Indeed. In this case I'd prefer if this option is added upstream. The --phpapi option is used to determine the path to the extensions directory. In the case of architectures where LFS is enabled, the phpapi value is appended '+lfs'. > > 100-recode_is_shared.patch > The conflict between MySQL and recode should only happen with an old > libmysql (3.23?) not sure about imap ... but in your case the patch > might make sense, while I won't directly apply it to our tree as > usually people will build extensions to load them together... I will later be commenting on this one. > > 101-sqlite_is_shared.patch > Not sure if we do any modifications to SQLite but in general we prefer > to use the bundled lib so users get the same behavior on all > platforms. We use the shared library to avoid extra work on security issues and general maintenance. > > 108-64_bit_datetime.patch > 116-posixness_fix.patch > Why is that needed on our system? - I never saw an issue about these > and if they are missing there should be build issues. I will later be commenting on these two. > > use_embedded_timezonedb.patch > Like with the SQLite note above we prefer to have the same behavior > over all platforms by using a common time database. > Same here, updating the tz data on PHP everytime would be a pain. And yes, at some point we used the PECL extension but was later dropped in favour of the above patch. > force_libmysqlclient_r.patch > Why are you using the reentrant version of this library which is > slower than the "regular" one? Did you consider mysqlnd? * New patch: force_libmysqlclient_r.patch, forcing the build system to link against the threadsafe libmysqlclient without having to enable the other zts features in php. This is required since the apr libraries are now linking against this as well and mysql exports the same symbols from both libraries. Thanks to Stefan Fritsch (closes: #469081). I don't think we are going to switch to mysqlnd as it would require more maintenance from our part and more work in case of security issues. > > 009_ob-memory-leaks.patch > Any test case? (While this looks good on first sight) I think there was one for the original patch (as you can see the patch was taken from php). Later, when we refreshed the patches it was still applying at a different part of the same file and a quick review demonstrated it looked correct. > > exif_read_data-segfault.patch > Any sample case for this? Any bug report? > Looks like somebody forgot to remove it, as the code is already upstream (and the context that can be seen is exactly the same check). This is the fix for CVE-2009-2687. > sybase-alias.patch > I have no idea about MS SQL and Sybase but "randomly" aliassing looks > bad to me. Since the mssql and sybase extensions both use freetds their internal behaviour is the same. As such, the aliases were added to preserve compatibility with applications using the sybase set of functions. > > strcmp_null-OnUpdateErrorLog.patch > You should at least ad an "echo 'done';" or such to the expect > section to make sure it really works ... Could be added, yes, but in case it fails it would lead to a segfault. > > I skipped the patches related to build system and such and maybe skipped > one or two by accident .... > Some of the other patches include: libdb_is_-ldb 115-autoconf_ftbfs.patch fix_broken_upstream_tests.patch bad_whatis_entries.patch Which should all be merged. Here's an online copy of the latest changelog: http://packages.debian.org/changelogs/pool/main/p/php5/current/changelog.html Cheers, -- Raphael Geissert - Debian Developer www.debian.org - get.debian.net --- ## #46755 — Jani Taskinen — 2010-01-16T21:18:44+00:00 URL: https://externals.io/message/46751#46755 · In reply to: #46754 16.1.2010 20:10, Raphael Geissert wrote: > Some of the other patches include: > libdb_is_-ldb Why? Potentially breaks things when you assume db/ being correct place.. > 115-autoconf_ftbfs.patch Hell no. You're breaking the configure again with this crap. I already reverted the idiocy once, don't even think about doing this shit again. PHP configure works properly only with autoconf-2.13 which was the last working autoconf. > fix_broken_upstream_tests.patch The soap thing seems ok. But why are you disabling ext/standard/tests/general_functions/phpinfo.phpt ?? That test passes fine for me everywhere. The patch says: test suite's handling of "%s" is incompatible with this test" Eh? > bad_whatis_entries.patch Looks ok. > Which should all be merged And next time, please include direct links to the patches, makes easier to follow these and comment on them. :) --Jani --- ## #46756 — Gwynne Raskind — 2010-01-16T22:05:56+00:00 URL: https://externals.io/message/46751#46756 · In reply to: #46755 > > […] > Hell no. You're breaking the configure again with this crap. I already reverted the idiocy once, don't even think about doing this shit again. PHP configure works properly only with autoconf-2.13 which was the last working autoconf. Which autoconf was the last "working" one is a matter of opinion at this point. The Autoconf people would love for us to stop using 2.13 - the only reason it still exists is because we use it. (Or so I'm told, anyway, I don't know this for a fact firsthand.) Personally, I'd rather see PHP go to CMake. CMake does have its own host of problems, but they could be dealt with. And yes, I would volunteer to revive the CMake effort. -- Gwynne --- ## #46757 — Alexey Zakhlestin — 2010-01-16T22:29:05+00:00 URL: https://externals.io/message/46751#46757 · In reply to: #46756 > > > […] > > Which autoconf was the last "working" one is a matter of opinion at this point. The Autoconf people would love for us to stop using 2.13 - the only reason it still exists is because we use it. (Or so I'm told, anyway, I don't know this for a fact firsthand.) Personally, I'd rather see PHP go to CMake. CMake does have its own host of problems, but they could be dealt with. And yes, I would volunteer to revive the CMake effort. strong +1 from me would be glad to help --- ## #46758 — Jani Taskinen — 2010-01-16T23:28:35+00:00 URL: https://externals.io/message/46751#46758 · In reply to: #46757 17.1.2010 0:29, Alexey Zakhlestin wrote: > > > > > […] > > strong +1 from me > would be glad to help Strong -1000 from me, there's nothing wrong with autoconf. --Jani --- ## #46760 — Raphael Geissert — 2010-01-17T03:19:49+00:00 URL: https://externals.io/message/46751#46760 · In reply to: #46755 Jani Taskinen wrote: > 16.1.2010 20:10, Raphael Geissert wrote: > > […] > > Why? Potentially breaks things when you assume db/ being correct place.. Do you have an example of any actual case? > > > […] > > Hell no. You're breaking the configure again with this crap. I already > reverted the idiocy once, don't even think about doing this shit again. > PHP configure works properly only with autoconf-2.13 which was the last > working autoconf. Can you tell me what exactly we are breaking? divert calls should only be used internally by autoconf and the, apparently useless, usage of them in php makes it fail to build with any other autoconf. > > > […] > > The soap thing seems ok. > > But why are you disabling > ext/standard/tests/general_functions/phpinfo.phpt ?? That test passes fine > for me everywhere. The patch says: > > test suite's handling of "%s" is incompatible with this test" > > Eh? I've never seen that test actually work but I don't remember the details right now. Will check it next time and report. > > > […] > > And next time, please include direct links to the patches, makes easier to > follow these and comment on them. :) Ok. Cheers, -- Raphael Geissert - Debian Developer www.debian.org - get.debian.net --- ## #46761 — Rasmus Lerdorf — 2010-01-17T03:56:51+00:00 URL: https://externals.io/message/46751#46761 · In reply to: #46760 Raphael Geissert wrote: > Can you tell me what exactly we are breaking? divert calls should only be > used internally by autoconf and the, apparently useless, usage of them in > php makes it fail to build with any other autoconf. Have a look in the archives. I tried getting the divert stuff working correctly and it broke a bunch of stuff. The current attempt is in HEAD, but it needs more work. And yes, the Debian patch the way it is now definitely won't work in the general case. -Rasmus --- ## #46774 — Jani Taskinen — 2010-01-18T08:51:06+00:00 URL: https://externals.io/message/46751#46774 · In reply to: #46760 > Jani Taskinen wrote: > > > […] > > Do you have an example of any actual case? Do you have an example where it's actually needed? > Can you tell me what exactly we are breaking? divert calls should only be > used internally by autoconf and the, apparently useless, usage of them in > php makes it fail to build with any other autoconf. If you remove them, things break. I don't remember right now why, but it did, Rasmus tried this already and failed. Search the mailing lists for more. His commit message is a bit weird, I think it should say "2.6x is broken in so many ways" rather than 2.13: http://svn.php.net/viewvc?view=revision&revision=291414 --Jani --- ## #46775 — Rasmus Lerdorf — 2010-01-18T09:12:07+00:00 URL: https://externals.io/message/46751#46775 · In reply to: #46774 Jani Taskinen wrote: > > > […] > > Do you have an example where it's actually needed? > > > […] > > If you remove them, things break. I don't remember right now why, but it > did, Rasmus tried this already and failed. Search the mailing lists for > more. His commit message is a bit weird, I think it should say "2.6x is > broken in so many ways" rather than 2.13: > > http://svn.php.net/viewvc?view=revision&revision=291414 Not really. It is only maintaining backward compatibility with 2.13 that is broken. In HEAD where we can drop that version entirely, things are fine. Since we can't do that in 5.3, I gave up on the changes for this branch. -Rasmus --- ## #46776 — Gwynne Raskind — 2010-01-18T09:14:46+00:00 URL: https://externals.io/message/46751#46776 · In reply to: #46774 > > […] > If you remove them, things break. I don't remember right now why, but it did, Rasmus tried this already and failed. Search the mailing lists for more. His commit message is a bit weird, I think it should say "2.6x is broken in so many ways" rather than 2.13: > > http://svn.php.net/viewvc?view=revision&revision=291414 If I remember right, removing the diverts makes the ./configure --help output no longer pretty. And 2.6x and 2.13 are both broken in their own lists of ways. Though I thought the use of high-numbered diversions was actually a supported thing - or was that only in 2.13? -- Gwynne --- ## #46783 — Raphael Geissert — 2010-01-18T21:00:11+00:00 URL: https://externals.io/message/46751#46783 · In reply to: #46774 Jani Taskinen wrote: > > > […] > > Do you have an example where it's actually needed? Yes, every time a new libdb version is released we would have to patch the config.m4 file. Instead, what this patch does is make the config.m4 pick the versionless files. For example, 5.3.1's ext/dba/config.m4 only knows up to db4.6 but at Debian we have already moved to 4.7 and are switching to 4.8. > > […] > > If you remove them, things break. I don't remember right now why, but it > did, Rasmus tried this already and failed. Search the mailing lists for > more. His commit message is a bit weird, I think it should say "2.6x is > broken in so many ways" rather than 2.13: > > http://svn.php.net/viewvc?view=revision&revision=291414 > Well, we use autoconf2.63 (2.13 is soon to be dropped) and we need those changes to make it work. Cheers, -- Raphael Geissert - Debian Developer www.debian.org - get.debian.net --- ## #46784 — Raphael Geissert — 2010-01-18T21:06:31+00:00 URL: https://externals.io/message/46751#46784 · In reply to: #46776 Gwynne Raskind wrote: > > Though I thought the use of high-numbered diversions was > actually a supported thing - or was that only in 2.13? > That argument is not supported by the autoconf manual. Please see the discussion at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=542906#10 Cheers, -- Raphael Geissert - Debian Developer www.debian.org - get.debian.net --- ## #46785 — Rasmus Lerdorf — 2010-01-18T21:20:54+00:00 URL: https://externals.io/message/46751#46785 · In reply to: #46784 Raphael Geissert wrote: > Gwynne Raskind wrote: > > […] > > That argument is not supported by the autoconf manual. Please see the > discussion at > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=542906#10 Well, the autoconf maintainers have a different opinion on that. They don't see anything wrong with doing diverts if there is no other way to accomplish something. You can read this thread about my attempts to create a portable autoconf setup that works both in autoconf-2.60+ and previous versions. There were various ideas, but in the end none of them proved to be reliable. We need diversions prior to 2.60 or whichever version introduced AC_PRESERVE_HELP_ORDER. In autoconf versions that have AC_PRESERVE_HELP_ORDER we can use that and we can drop the diversions entirely. Renumbering the diversions and the various other attempts people have made doesn't work. -Rasmus -Rasmus --- ## #46786 — Rasmus Lerdorf — 2010-01-18T21:21:39+00:00 URL: https://externals.io/message/46751#46786 · In reply to: #46785 Rasmus Lerdorf wrote: > Raphael Geissert wrote: > > […] > > Well, the autoconf maintainers have a different opinion on that. They > don't see anything wrong with doing diverts if there is no other way to > accomplish something. > > You can read this thread about my attempts to create a portable autoconf > setup that works both in autoconf-2.60+ and previous versions. There > were various ideas, but in the end none of them proved to be reliable. > > We need diversions prior to 2.60 or whichever version introduced > AC_PRESERVE_HELP_ORDER. In autoconf versions that have > AC_PRESERVE_HELP_ORDER we can use that and we can drop the diversions > entirely. > > Renumbering the diversions and the various other attempts people have > made doesn't work. Oops, missed the thread link: http://lists.gnu.org/archive/html/autoconf/2009-11/msg00100.html --- ## #46787 — Raphael Geissert — 2010-01-18T21:45:51+00:00 URL: https://externals.io/message/46751#46787 · In reply to: #46786 Rasmus Lerdorf wrote: > Rasmus Lerdorf wrote: > > […] > > Oops, missed the thread link: > > http://lists.gnu.org/archive/html/autoconf/2009-11/msg00100.html > I see, thanks for the pointer. Cheers, -- Raphael Geissert - Debian Developer www.debian.org - get.debian.net --- ## #47795 — Raphael Geissert — 2010-04-05T18:11:05+00:00 URL: https://externals.io/message/46751#47795 · In reply to: #46754 Raphael Geissert wrote: > Johannes Schlüter wrote: > > Although I would have preferred you wait for me to submit each patch > individually with enough information (because I've only been like two > years co-maintaining the packages and most patches were added by others, > and most before I joined), thanks. I'll try to comment on each but I don't > know about them all and can't check them right now (I'm short on time). > I haven't had much time to work on merging those patches, but will try to eventually get to do it. > > […] The least I can say is that that patch is obsolete. We don't build the apache modules with ZTS. However, I don't understand what you are saying about Gtk. The patch doesn't change the behaviour of the cli/cgi/embed SAPIs. It only re-enables `dl()` on any SAPI even with ZTS. > > […] The main issue is that we build the different SAPIs with different configure flags. This seems to have lead to bogus bug reports in the past and confusion (--disable-*/--without-* in certain SAPIs). For example, the shared extensions are built when building the apache SAPI, the cgi build options are also massaged after a first build, etc. > > […] > > Indeed. In this case I'd prefer if this option is added upstream. > The --phpapi option is used to determine the path to the extensions > directory. In the case of architectures where LFS is enabled, the phpapi > value is appended '+lfs'. Is there any objection to making this change? (only to trunk, I assume.) > > […] Shouldn't checking for $PHP_IMAP_SHARED != "yes" (and the same for mysql) work? if either recode or imap/mysql are being built as shared I don't think there's any reason to abort the build. A warning should be enough, don't you think? > > […] > > I will later be commenting on these two. Haven't had time to check, deferring. > > […] Leaving any security concern aside, there are at the moment some missing features and behaviour changes that I don't feel very comfortable with. In a future release we might switch, but won't happen for Squeeze. > Some of the other patches include: > libdb_is_-ldb Not yet merged, but at least support for db-4.{7,8} was added. > 115-autoconf_ftbfs.patch as per discussion we will continue carrying it > fix_broken_upstream_tests.patch not completely merged/resolved > bad_whatis_entries.patch merged Cheers, -- Raphael Geissert --- ## #47884 — Derick Rethans — 2010-04-12T08:52:27+00:00 URL: https://externals.io/message/46751#47884 · In reply to: #46754 > > […] > > Same here, updating the tz data on PHP everytime would be a pain. And yes, > at some point we used the PECL extension but was later dropped in favour of > the above patch. Please reinstate the PECL extension approach. This can go to volatile just like the tzdata package. regards, Derick -- http://derickrethans.nl | http://xdebug.org Like Xdebug? Consider a donation: http://xdebug.org/donate.php twitter: @derickr and @xdebug --- ## #52372 — Ferenc Kovacs — 2011-05-15T01:58:52+00:00 URL: https://externals.io/message/46751#52372 · In reply to: #47795 > > > > […] > > Shouldn't checking for $PHP_IMAP_SHARED != "yes" (and the same for mysql) > work? if either recode or imap/mysql are being built as shared I don't > think > there's any reason to abort the build. A warning should be enough, don't > you > think? > > Hi. I've just run into this issue. with debian (and dotdeb) one can have both recode and imap configured, with vanilla trunk, I get an "configure: error: recode extension can not be configured together with: imap" which is in sync with the docs: http://www.php.net/manual/en/recode.installation.php "The IMAP, recode, YAZ and Cyrus extensions cannot be used in conjuction, because they share the same internal symbols. Note: Yaz 2.0 and above does not suffer from this problem." it seems that the guys at fedora also "fixed" this: http://osdir.com/ml/fedora-extras-commits/2009-02/msg01651.html could somebody look into this? I think we should either enable this (throw a warning not an error), or we should tell the distros to don't do this. Tyrael