Debian PHP patches View as Markdown

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