Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:55499 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 13360 invoked from network); 18 Sep 2011 15:12:46 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 18 Sep 2011 15:12:46 -0000 Authentication-Results: pb1.pair.com smtp.mail=abrender@elitehosts.com; spf=permerror; sender-id=unknown Authentication-Results: pb1.pair.com header.from=abrender@elitehosts.com; sender-id=unknown Received-SPF: error (pb1.pair.com: domain elitehosts.com from 209.67.29.110 cause and error) X-PHP-List-Original-Sender: abrender@elitehosts.com X-Host-Fingerprint: 209.67.29.110 webmail.elitehosts.com Received: from [209.67.29.110] ([209.67.29.110:37657] helo=ezri.elitehosts.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id 2C/53-22692-D6A067E4 for ; Sun, 18 Sep 2011 11:12:46 -0400 Received: from al.secure.elitehosts.com ([209.67.23.117] helo=[192.168.40.6]) by ezri.elitehosts.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.72) (envelope-from ) id 1R5J2v-0000qw-Lo for internals@lists.php.net; Sun, 18 Sep 2011 11:12:42 -0400 Message-ID: <4E760A6A.4080308@elitehosts.com> Date: Sun, 18 Sep 2011 11:12:42 -0400 Organization: Elite Hosts User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.21) Gecko/20110831 Thunderbird/3.1.13 MIME-Version: 1.0 To: internals@lists.php.net References: <4E6B9466.6060608@elitehosts.com> <4E6D1340.6010408@elitehosts.com> <4E6D5263.60509@elitehosts.com> In-Reply-To: Content-Type: multipart/mixed; boundary="------------060906070904000207080105" Subject: Re: [PHP-DEV] [PATCH]#55651 From: abrender@elitehosts.com (Avi Brender) --------------060906070904000207080105 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit On 09/12/2011 05:54 AM, Pierre Joye wrote: > 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. 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. --------------060906070904000207080105 Content-Type: text/x-patch; name="ftp_usepasvaddress.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="ftp_usepasvaddress.patch" diff -NaurbB ext/ftp.orig/ftp.c ext/ftp/ftp.c --- ext/ftp.orig/ftp.c 2010-12-31 21:19:59.000000000 -0500 +++ ext/ftp/ftp.c 2011-09-09 03:51:08.000000000 -0400 @@ -712,10 +712,11 @@ memset(&ftp->pasvaddr, 0, n); sa = (struct sockaddr *) &ftp->pasvaddr; -#if HAVE_IPV6 if (getpeername(ftp->fd, sa, &n) < 0) { return 0; } + +#if HAVE_IPV6 if (sa->sa_family == AF_INET6) { struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *) sa; char *endptr, delimiter; @@ -768,8 +769,9 @@ ipbox.c[n] = (unsigned char) b[n]; } sin = (struct sockaddr_in *) sa; - sin->sin_family = AF_INET; + if (ftp->usepasvaddress) { sin->sin_addr = ipbox.ia[0]; + } sin->sin_port = ipbox.s[2]; ftp->pasv = 2; diff -NaurbB ext/ftp.orig/ftp.h ext/ftp/ftp.h --- ext/ftp.orig/ftp.h 2010-12-31 21:19:59.000000000 -0500 +++ ext/ftp/ftp.h 2011-09-09 03:52:38.000000000 -0400 @@ -31,6 +31,7 @@ #define FTP_DEFAULT_TIMEOUT 90 #define FTP_DEFAULT_AUTOSEEK 1 +#define FTP_DEFAULT_USEPASVADDRESS 1 #define PHP_FTP_FAILED 0 #define PHP_FTP_FINISHED 1 #define PHP_FTP_MOREDATA 2 @@ -71,6 +72,7 @@ php_sockaddr_storage pasvaddr; /* passive mode address */ long timeout_sec; /* User configureable timeout (seconds) */ int autoseek; /* User configureable autoseek flag */ + int usepasvaddress; /* Use the address returned by the pasv command */ int nb; /* "nonblocking" transfer in progress */ databuf_t *data; /* Data connection for "nonblocking" transfers */ diff -NaurbB ext/ftp.orig/php_ftp.c ext/ftp/php_ftp.c --- ext/ftp.orig/php_ftp.c 2010-12-31 21:19:59.000000000 -0500 +++ ext/ftp/php_ftp.c 2011-09-11 19:11:32.000000000 -0400 @@ -315,6 +315,7 @@ REGISTER_LONG_CONSTANT("FTP_AUTORESUME", PHP_FTP_AUTORESUME, CONST_PERSISTENT | CONST_CS); 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); REGISTER_LONG_CONSTANT("FTP_FAILED", PHP_FTP_FAILED, CONST_PERSISTENT | CONST_CS); REGISTER_LONG_CONSTANT("FTP_FINISHED", PHP_FTP_FINISHED, CONST_PERSISTENT | CONST_CS); REGISTER_LONG_CONSTANT("FTP_MOREDATA", PHP_FTP_MOREDATA, CONST_PERSISTENT | CONST_CS); @@ -363,6 +364,7 @@ /* autoseek for resuming */ ftp->autoseek = FTP_DEFAULT_AUTOSEEK; + ftp->usepasvaddress = FTP_DEFAULT_USEPASVADDRESS; #if HAVE_OPENSSL_EXT /* disable ssl */ ftp->use_ssl = 0; @@ -399,6 +401,7 @@ /* autoseek for resuming */ ftp->autoseek = FTP_DEFAULT_AUTOSEEK; + ftp->usepasvaddress = FTP_DEFAULT_USEPASVADDRESS; /* enable ssl */ ftp->use_ssl = 1; @@ -1395,6 +1398,13 @@ ftp->autoseek = Z_LVAL_P(z_value); RETURN_TRUE; break; + case PHP_FTP_OPT_USEPASVADDRESS: + if (Z_TYPE_P(z_value) != IS_BOOL && Z_TYPE_P(z_value) != IS_LONG) { + convert_to_boolean_ex(&z_value); + } + ftp->usepasvaddress = Z_LVAL_P(z_value); + RETURN_TRUE; + break; default: php_error_docref(NULL TSRMLS_CC, E_WARNING, "Unknown option '%ld'", option); RETURN_FALSE; @@ -1424,6 +1434,9 @@ case PHP_FTP_OPT_AUTOSEEK: RETURN_BOOL(ftp->autoseek); break; + case PHP_FTP_OPT_USEPASVADDRESS: + RETURN_BOOL(ftp->usepasvaddress); + break; default: php_error_docref(NULL TSRMLS_CC, E_WARNING, "Unknown option '%ld'", option); RETURN_FALSE; diff -NaurbB ext/ftp.orig/php_ftp.h ext/ftp/php_ftp.h --- ext/ftp.orig/php_ftp.h 2010-12-31 21:19:59.000000000 -0500 +++ ext/ftp/php_ftp.h 2011-09-09 02:59:44.000000000 -0400 @@ -29,6 +29,7 @@ #define PHP_FTP_OPT_TIMEOUT_SEC 0 #define PHP_FTP_OPT_AUTOSEEK 1 +#define PHP_FTP_OPT_USEPASVADDRESS 2 #define PHP_FTP_AUTORESUME -1 PHP_MINIT_FUNCTION(ftp); diff -NaurbB ext/ftp.orig/tests/ftp_usepasvaddress.phpt ext/ftp/tests/ftp_usepasvaddress.phpt --- ext/ftp.orig/tests/ftp_usepasvaddress.phpt 1969-12-31 19:00:00.000000000 -0500 +++ ext/ftp/tests/ftp_usepasvaddress.phpt 2011-09-18 11:06:40.000000000 -0400 @@ -0,0 +1,38 @@ +--TEST-- +FTP FTP_USEPASVADDRESS +--SKIPIF-- + +--FILE-- + +--EXPECT-- +bool(true) +bool(true) +bool(true) +bool(true) +bool(true) +bool(true) +bool(false) +bool(true) +bool(false) --------------060906070904000207080105--