# [PATCH]#55651 - Thread: https://externals.io/message/55354 - Original archive: https://news-web.php.net/php.internals/55354 - Started by Avi Brender on 2011-09-10T16:46:30+00:00 - Messages: 11 - 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 --- ## #55354 — Avi Brender — 2011-09-10T16:46:30+00:00 URL: https://externals.io/message/55354#55354 Hi, I've submitted bug #55651 along with a patch to implement a fix (also attached) for the passive FTP mode issue. I was hoping that someone could review the bug report and consider the patch for inclusion in future PHP releases. Thanks so much! Avi Brender Elite Hosts, Inc www.elitehosts.com --- ## #55356 — Laruence — 2011-09-11T03:59:21+00:00 URL: https://externals.io/message/55354#55356 · In reply to: #55354 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 : > Hi, > > I've submitted bug #55651 along with a patch to implement a fix (also > attached) for the passive FTP mode issue. I was hoping that someone could > review the bug report and consider the patch for inclusion in future PHP > releases. > > Thanks so much! > > Avi Brender > Elite Hosts, Inc > www.elitehosts.com > > -- > > > -- Laruence  Xinchen Hui http://www.laruence.com/ --- ## #55363 — Pierre Joye — 2011-09-11T14:53:46+00:00 URL: https://externals.io/message/55354#55363 · In reply to: #55356 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: >    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 : > > […] > > > > -- > Laruence  Xinchen Hui > http://www.laruence.com/ > > -- > > > > -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #55366 — Avi Brender — 2011-09-11T20:00:00+00:00 URL: https://externals.io/message/55354#55366 · In reply to: #55363 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. > 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. > > > > […] could > > […] future PHP > > […] --- ## #55367 — Pierre Joye — 2011-09-11T20:24:04+00:00 URL: https://externals.io/message/55354#55367 · In reply to: #55366 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, > > 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. > > > > […] > > > -- > > > -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #55375 — Avi Brender — 2011-09-12T00:29:23+00:00 URL: https://externals.io/message/55354#55375 · In reply to: #55367 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. > 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, > > > > […] --- ## #55376 — Laruence — 2011-09-12T04:29:30+00:00 URL: https://externals.io/message/55354#55376 · In reply to: #55375 2011/9/12 Avi Brender : > 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 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 > 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. > > > > > […] > > -- > > > -- Laruence  Xinchen Hui http://www.laruence.com/ --- ## #55377 — Laruence — 2011-09-12T04:45:07+00:00 URL: https://externals.io/message/55354#55377 · In reply to: #55376 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 : > 2011/9/12 Avi Brender : > > […] > 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 > > […] > > > > -- > Laruence  Xinchen Hui > http://www.laruence.com/ > -- Laruence  Xinchen Hui http://www.laruence.com/ --- ## #55378 — Avi Brender — 2011-09-12T05:00:21+00:00 URL: https://externals.io/message/55354#55378 · In reply to: #55377 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. > 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: > > […] --- ## #55380 — Pierre Joye — 2011-09-12T09:54:22+00:00 URL: https://externals.io/message/55354#55380 · In reply to: #55375 hi! > 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. + 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 :) > 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. 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, -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #55499 — Avi Brender — 2011-09-18T15:12:42+00:00 URL: https://externals.io/message/55354#55499 · In reply to: #55380 > hi! > > > > […] > + REGISTER_LONG_CONSTANT("FTP_USEPASVADDRESS", > PHP_FTP_OPT_USEPASVADDRESS, CONST_PERSISTENT | CONST_CS); > > It is a userland constant too. 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); > Everyone understands `FTP_AUTORESUME` or FTP_TIMEOUT_SEC, > _FTP_OPT_USEPASVADDRESS is cryptic compared to the other :) 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` > > […] > That's fine too. You can the tests with the patch too, just do svn add > ext/ftp/tests/... before you call svn diff. Attached is the patch with the updated code and the test. > Thanks for your work so far! > > Cheers, 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.