FAQ
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

Search Discussions

  • Laruence at Sep 11, 2011 at 3:59 am
    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 <abrender@elitehosts.com>:
    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

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


    --
    Laruence  Xinchen Hui
    http://www.laruence.com/
  • Pierre Joye at Sep 11, 2011 at 2:53 pm
    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.
    On Sun, Sep 11, 2011 at 5:59 AM, Laruence wrote:
    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 <abrender@elitehosts.com>:
    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

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


    --
    Laruence  Xinchen Hui
    http://www.laruence.com/

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


    --
    Pierre

    @pierrejoye | http://blog.thepimp.net | http://www.libgd.org
  • Avi Brender at Sep 11, 2011 at 8:00 pm
    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.
    On 09/11/2011 10:53 AM, Pierre Joye wrote:
    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.
    >
    On Sun, Sep 11, 2011 at 5:59 AM, Laruence wrote:
    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 <abrender@elitehosts.com>:
    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
    >>>
    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    >>>
    >>
    >>
    >>
    --
    Laruence Xinchen Hui
    http://www.laruence.com/
    >>
    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    >>
    >>
    >
    >
    >
  • Pierre Joye at Sep 11, 2011 at 8:24 pm
    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,
    On Sun, Sep 11, 2011 at 10:00 PM, Avi Brender wrote:
    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.
    On 09/11/2011 10:53 AM, Pierre Joye wrote:
    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.
    On Sun, Sep 11, 2011 at 5:59 AM, Laruence wrote:
    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 <abrender@elitehosts.com>:
    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

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


    --
    Laruence  Xinchen Hui
    http://www.laruence.com/

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


    --
    Pierre

    @pierrejoye | http://blog.thepimp.net | http://www.libgd.org
  • Avi Brender at Sep 12, 2011 at 12:29 am
    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.

    On 09/11/2011 04:24 PM, Pierre Joye wrote:
    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,

    On Sun, Sep 11, 2011 at 10:00 PM, Avi Brenderwrote:
    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.
    On 09/11/2011 10:53 AM, Pierre Joye wrote:
    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.

    On Sun, Sep 11, 2011 at 5:59 AM, Laruencewrote:
    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<abrender@elitehosts.com>:
    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

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php

    --
    Laruence Xinchen Hui
    http://www.laruence.com/

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
  • Laruence at Sep 12, 2011 at 4:29 am

    2011/9/12 Avi Brender <abrender@elitehosts.com>:
    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.

    On 09/11/2011 04:24 PM, Pierre Joye wrote:

    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,

    On Sun, Sep 11, 2011 at 10:00 PM, Avi Brender<abrender@elitehosts.com>
    wrote:
    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.
    On 09/11/2011 10:53 AM, Pierre Joye wrote:

    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.

    On Sun, Sep 11, 2011 at 5:59 AM, Laruencewrote:
    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<abrender@elitehosts.com>:
    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

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php

    --
    Laruence  Xinchen Hui
    http://www.laruence.com/

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


    --
    Laruence  Xinchen Hui
    http://www.laruence.com/
  • Laruence at Sep 12, 2011 at 4:45 am
    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 <laruence@php.net>:
    2011/9/12 Avi Brender <abrender@elitehosts.com>:
    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.

    On 09/11/2011 04:24 PM, Pierre Joye wrote:

    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,

    On Sun, Sep 11, 2011 at 10:00 PM, Avi Brender<abrender@elitehosts.com>
    wrote:
    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.
    On 09/11/2011 10:53 AM, Pierre Joye wrote:

    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.

    On Sun, Sep 11, 2011 at 5:59 AM, Laruencewrote:
    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<abrender@elitehosts.com>:
    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

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php

    --
    Laruence  Xinchen Hui
    http://www.laruence.com/

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


    --
    Laruence  Xinchen Hui
    http://www.laruence.com/


    --
    Laruence  Xinchen Hui
    http://www.laruence.com/
  • Avi Brender at Sep 12, 2011 at 5:00 am
    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.
    On 09/12/2011 12:45 AM, Laruence wrote:
    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<laruence@php.net>:
    2011/9/12 Avi Brender<abrender@elitehosts.com>:
    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.

    On 09/11/2011 04:24 PM, Pierre Joye wrote:
    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,

    On Sun, Sep 11, 2011 at 10:00 PM, Avi Brender<abrender@elitehosts.com>
    wrote:
    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.
    On 09/11/2011 10:53 AM, Pierre Joye wrote:
    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.

    On Sun, Sep 11, 2011 at 5:59 AM, Laruencewrote:
    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<abrender@elitehosts.com>:
    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

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    --
    Laruence Xinchen Hui
    http://www.laruence.com/

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php

    --
    Laruence Xinchen Hui
    http://www.laruence.com/
  • Pierre Joye at Sep 12, 2011 at 9:54 am
    hi!
    On Mon, Sep 12, 2011 at 2:29 AM, Avi Brender wrote:
    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,
  • Avi Brender at Sep 18, 2011 at 3:12 pm

    On 09/12/2011 05:54 AM, Pierre Joye wrote:
    hi!

    On Mon, Sep 12, 2011 at 2:29 AM, Avi Brenderwrote:
    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.
    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
    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.
    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.
  • Avi Brender at Sep 11, 2011 at 4:56 am
    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
    On 09/10/2011 11:59 PM, Laruence wrote:
    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<abrender@elitehosts.com>:
    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

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedSep 10, '11 at 5:01p
activeSep 18, '11 at 3:12p
posts12
users3
websitephp.net

People

Translate

site design / logo © 2022 Grokbase