[PHP-INTERNALS] [PATCH]#55651

Hi:
after a quick look, I have one suggestion,
if the (Z_TYPE_P(z_value) != IS_BOOL), you should call
convert_to_boolean_ex to convert it to a boolean
otherwise, people can not use a interge 1 as a true flag.

thanks

2011/9/11 Avi Brender <[email protected]>:

hi,

A simple test if it is IS_BOOL or IS_LONG should be enough, both types
use the the long value (convert_to_boolean_ex is slow and duplicate
the zval while it is not necessary). Then test > 0 instead of simply
assigning the value.

Hi,

I've updated the patch - please see attached.

Avi Brender
Elite Hosts, Inc
www.elitehosts.com
------------------------------------------------
WARNING !!! This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged information.
Any unauthorized review; use, disclosure or distribution is prohibited,
and could result in criminal prosecution. If you are not the intended
recipient, please contact the sender by reply email and destroy all
copies of the original message. This message is private and is
considered a confidential exchange - public disclosure of this
electronic message or its contents are prohibited.
>
>>
>>
>>>
could
future PHP
>>>
>>>
>>>
>>>
>>
>>
>>
>>
>>
>>
>
>
>

hi!

Please upload the patch in the bug tracker as well.

It would be also better to use a more verbose name.
FTP_OPT_USEPASVADDRESS is somehow cryptic.

Laruence's comment is still valid, the zval should be converted if it
is not int or bool.

Btw, could you test cases as well please?

Cheers,

Hi,

Please see if the attached patch better addresses your concerns.

Regarding the variable name, the PHP_FTP_OPT_USEPASVADDRESS is only
internal and is modeled after the other variables PHP_FTP_TIMEOUT_SEC
and PHP_FTP_OPT_AUTOSEEK. The variable actually passed to the
ftp_set_option() function by users is FTP_USEPASVADDRESS as defined in
php_ftp.c which is inline with the other variables FTP_AUTORESUME,
FTP_TIMEOUT_SEC, FTP_AUTOSEEK etc.

In terms of tests, what type of tests were you thinking of? We can't
ensure that ftp->pasvaddr is set properly in response to a PASV command
unless there's a way to expose those internal variables to the test -
I'm not familiar enough with the internal PHP code to know if that's
possible. If you're referring to tests that ensure that 1/0 true/false
values passed to ftp_set_option() work properly then I've attached a
test file for that.

I don't want to clutter the bugzilla ticket with many attachments so
once the patch is approved I will post the final version in the ticket
if that's okay.

All the best,

Avi Brender
Elite Hosts, Inc
www.elitehosts.com
------------------------------------------------
WARNING !!! This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged information.
Any unauthorized review; use, disclosure or distribution is prohibited,
and could result in criminal prosecution. If you are not the intended
recipient, please contact the sender by reply email and destroy all
copies of the original message. This message is private and is
considered a confidential exchange - public disclosure of this
electronic message or its contents are prohibited.

actually I think it can, plz refer to the existing test config script
"server.inc" under the ftp/tests/,
and maybe you can simulate a test environ?

and thanks for your work on PHP

hmm, after deep looking, I found maybe this behavior(cann't connect
to NAT server when use pasv mode) is "as expected",
since this is not php ftp-ext issue, but a ftp server configure issue,

that is, if we apply this patch, will make the php ftp-ext not a
standard ftp protocl executor.


thanks

2011/9/12 Laruence <[email protected]>:

I definitely agree that the problem is with a mis-configured FTP server
returning a private RFC1918 IP address. That would happen if the FTP
server was on a server behind a NAT gateway and the server only knew
about it's local RFC1918 IP address and not it's internet-routable IP
address. However, every other FTP client (including firezilla, wget and
the CURL module in PHP) has no problem automatically detecting this and
connecting to the IP address specified when connecting and not the one
returned by the server in response to PASV. The FTP_USEPASVADDRESS
setting would in fact cause the ftp extension to ignore the IP address
returned by the server, however the default setting to that setting is
TRUE which means that any existing code or future code using the ftp
module will behave exactly as it does now. However, manually setting
this setting to FALSE will help developers get around misconfigured FTP
servers as virtually all other FTP clients (including php's curl
extension) do. I found the need for this patch while debugging a script
that our customer was running that was trying to connect to an FTP
server in their office (described at
http://www.elitehosts.com/blog/php-ftp-passive-ftp-server-behind-nat-nightmare/)
so I believe that there is a real-world use case for this patch.

Yes, we're possibly allowing users to violate the protocol in order to
successfully communicate with the FTP server, but only if they knowingly
and explicitely set the variable FTP_USEPASVADDRESS to false, in which
case it is safe to assume that the developer is doing it for a reason.


Avi Brender
Elite Hosts, Inc
www.elitehosts.com

------------------------------------------------
WARNING !!! This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged information.
Any unauthorized review; use, disclosure or distribution is prohibited,
and could result in criminal prosecution. If you are not the intended
recipient, please contact the sender by reply email and destroy all
copies of the original message. This message is private and is
considered a confidential exchange - public disclosure of this
electronic message or its contents are prohibited.

hi!
+ REGISTER_LONG_CONSTANT("FTP_USEPASVADDRESS",
PHP_FTP_OPT_USEPASVADDRESS, CONST_PERSISTENT | CONST_CS);

It is a userland constant too.

Everyone understands FTP_AUTORESUME or FTP_TIMEOUT_SEC,
_FTP_OPT_USEPASVADDRESS is cryptic compared to the other :)
That's fine too. You can the tests with the patch too, just do svn add
ext/ftp/tests/... before you call svn diff.

Thanks for your work so far!

Cheers,

I'm simply following the other code in php_ftp.c - it's modeled after
the other existing options passed to ftp_set_option - FTP_TIMEOUT_SEC
and FTP_AUTOSEEK, both of which define constants PHP_FTP_OPT_TIMEOUT_SEC
and PHP_FTP_OPT_AUTOSEEK
REGISTER_LONG_CONSTANT("FTP_TIMEOUT_SEC",
PHP_FTP_OPT_TIMEOUT_SEC, CONST_PERSISTENT | CONST_CS);
REGISTER_LONG_CONSTANT("FTP_AUTOSEEK", PHP_FTP_OPT_AUTOSEEK,
CONST_PERSISTENT | CONST_CS);
REGISTER_LONG_CONSTANT("FTP_USEPASVADDRESS",
PHP_FTP_OPT_USEPASVADDRESS, CONST_PERSISTENT | CONST_CS);
I'm not married to 'USEPASVADDRESS', but I can't think of any other
name. If you have any suggestions then please let me know. While it
might be a little cryptic, I think it perfectly describes what it does
and is no more cryptic than other defined constants such as FTP_MOREDATA
Avi Brender
Elite Hosts Inc
www.elitehosts.com

------------------------------------------------
WARNING !!! This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged information.
Any unauthorized review; use, disclosure or distribution is prohibited,
and could result in criminal prosecution. If you are not the intended
recipient, please contact the sender by reply email and destroy all
copies of the original message. This message is private and is
considered a confidential exchange - public disclosure of this
electronic message or its contents are prohibited.

Hi Laruence,

Sure thing - I was just using the above code that verified the AUTOSEEK option as a template. I've changed it to use convert_to_boolean_ex as per your suggestion :)

Thanks!

Avi Brender
Elite Hosts, Inc
www.elitehosts.com