First fixes for a 64bit compatible ftp extension

[m0ppers]

The ftp extension is old and does not support 64 bit. Pull request to start discussion and first fixes (testsuite works, my usecase works as well)

[weltling]

I don't quite get what do you mean with "64 bit" support as what i see in the patch is a fix for the resume functionality in the background ftp ext lib. As one can see here http://php.net/ftp_get the resumepos param comes as long from the userspace but is int in the ftp lib. Could you please explain that 64 bit part closer?

Also I'd say checks like

if (resumepos > 2147483647) {

should stay in the code. However they might need to be made more fine graned for specific platforms .

[m0ppers]

Ok "64 bit compatible" was wrong ;) what i meant was that the ftp extension fails to handle large file operations due to 32 bit restrictions. I have added a few testcases for some of the fails i have seen

[weltling]

Hi,

tried on windows 32 bit, it compiles fine. Also tried on CentOS 32 bit - here are some issues.

• Test ext/ftp/tests/filesize_large.phpt fails, here's the diff

001+ int(2147483647)
001- int(5368709120)

Obviously expected a number in the 64 bit range which fails on 32 bit. Therefore i've meant that the range checks have to stay in the code. Where "long" on 64bit linux is 8 byte, on 64bit windows it's still 4 byte. You could pack them into a macros. Of course in this particular test you only read size, not the file, but that's just a show case why the range checks should stay. This test should be duplicated for 32 and 64 bit, i'd suggest.

• Test ext/ftp/tests/ftp_nb_get_large.phpt fails, here's the diff.

001+ string(1) "Y"
002+ int(1073741824)
001- string(1) "X"
002- int(5368709120)
003+ /home/weltling/dws/src/m0ppers/main/streams/streams.c(530) : Stream of type 'STDIO' 0xb77cce6c (path:/home/weltling/dws/src/m0ppers/ext/ftp/tests/localfile.txt) was not closed
004+ [Mon Oct 22 15:54:53 2012] Script: '/home/weltling/dws/src/m0ppers/eaxt/ftp/tests/ftp_nb_get_large.php'
005+ /home/weltling/dws/src/m0ppers/main/streams/streams.c(292) : Freeing 0xB77CCE6C (140 bytes), script=/home/weltling/dws/src/m0ppers/ext/ftp/tests/ftp_nb_get_large.php
006+ /home/weltling/dws/src/m0ppers/ext/ftp/php_ftp.c(938) : Actual location (location was relayed)
007+ === Total 1 memory leaks detected ===

Here's additionally a leak.

I've got no 64bit VM at hand right now, will get one and test there later too. Tests can't run on windows (unfortunately) as they depend on pcntl ((

Cheers

[m0ppers]

The checks which were implemented were not working anyway. The original implementation contained ints in the function definitions and the checks couldn't work as you can't check for

int > MAX_INT

That's why i never saw this error when i hit the problem. So i consider them useless. We would have to implement specific checks with ulong etc for this to work. I don't think however that this is really needed and i don't think this is done in many places in the php source code, is it?

My patch however would enable 64 bit non windows systems to download large files and i think 90% of the (production) systems where php is running would benefit from it.

So i would add skipifs to my ftp testcases for 32 bit systems, try to find the leak and that would be all. What do you think?

[weltling]

Hi,

that sounds like a plan.

Regarding the range checks - in main/php.h there is

#ifndef LONG_MAX
#define LONG_MAX 2147483647L
#endif

Under linux it's defined in the /usr/include/limits.h each platform. So theoretically it'd be possible to check against LONG_MAX.

That checks come from the times where php really had int, not long as underlaying integer type. Nowadays when it comes from the user land it'd be always < LONG_MAX and > LONG_MIN. So that's ok to ommit that checks.

[pierrejoye]

LONG_MAX is defined at compile time. See http://lxr.php.net/xref/PHP_5_4/ext/gd/libgd/gd_security.c#4 for a correct check, with awareness of the correct int size. Similar code could be used using long.

[smalyshev]

@m0ppers any news on updating the patch?

[m0ppers]

Found the leak, skipped the large test on 32 bit platforms and merged with master. Should be ok now? I don't see how i could implement an overflow check here...the longs are coming directly from userspace...the example checks are done on allocation...however its too late at that point :S the only possible check is (and was) already implemented: if (resumepos > 0)

[php-pulls]

Comment on behalf of stas at php.net:

merged