28874 - IPv6 needs FTP to support EPSV command

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P3)

Description

[John G. Myers]

The FTP client code needs to support the EPSV command on IPv6 connections.

Comment 1

[John G. Myers]

EPSV is specified in RFC 2428.

Comment 2

[John G. Myers]

Attachment: Proposed fix

Comment 3

[John G. Myers]

Fix is attached.  Put on M16/nsbeta2 radar, assign to module owner for review.

Comment 4

[Gagan]

jud should review the fix. 

Comment 5

[Wan-Teh Chang]

I reviewed jgmyers' patch (id=8309) and thought
it is good and correct.  Here are some suggested
changes.

1. Capitalize method name 'serverIsIPv6'.
2. In the comments before nsFtpConnectionThread::serverIsIPv6,
   'cPipe' should be 'mCPipe'.  The argument 'hostAddr' is
   referred to as optional but it is not declared that way
   (with a default value).
3. In nsFtpConnectionThread::R_pasv(), the while loop
        while (*ptr >= 0 && *ptr <= 9) {
            port = port * 10 + *ptr++ - '0';
        }
   can be changed to a do-while loop because the
   condition is guaranteed to be true in the first
   pass.
4. The serverIsIPv6 method is invoked once in S_pasv()
   and once in R_pasv().  The second call to serverIsIPv6
   is wasteful because it's going to return the same result.
   S_pasv() could save the return value of serverIsIPv6 and
   hostAddr in class members and then R_pasv() would just
   use the saved values.  We could even do this as soon
   as mCPipe is set up because the server's address and
   whether it is IPv6 only depend on mCPipe.

Comment 6

[John G. Myers]

For (2) I'm simply removing 'cPipe'.  This was a function argument in an earlier 
form of the patch.  hostAddr is optional in the C sense, I can change this.

Valeski, would you prefer I move the ServerIsIPv6() code into the 
FTP_COMMAND_CONNECT: case of nsFtpConnectionThread::Process(), adding a 
private mIPv6ServerAddress member, or keep a private ServerIsIPv6() method which 
is called twice per data transfer?

Comment 7

[Judson Valeski]

thanks for reviewing it Wan-teh.

John, I think we should keep a member variable too. I would indeed push the 
isIPV6 logic into the command connect.

Comment 8

[John G. Myers]

Attachment: Revised proposed fix

Comment 9

[John G. Myers]

I can't put the logic in the command connect because the underlying socket is 
not connected until after we've started waiting for the banner.

Comment 10

[John G. Myers]

Attachment: Revised proposed fix (fix constructor)

Comment 11

[Wan-Teh Chang]

I reviewed the revised proposed patch (id=8448).

1. In S_pasv(), if mIPv6Checked is PR_FALSE, we
   set mIPv6Checked to PR_TRUE and initialize
   mIPv6ServerAddress to 0.  It would be a good
   idea to assert (use a fatal assertion such
   as PR_ASSERT) that mIPv6ServerAddress == 0,
   because if mIPv6ServerAddress is not null
   it will be leaked.

2. I think it's not necessary to free mIPv6ServerAddress
   and set it to 0 in StopProcessing(), at least it seems
   wrong to not also set mIPv6Checked to PR_FALSE.

   It seems that a better place to free mIPv6ServerAddress
   and set it to 0 would be FTP_COMMAND_CONNECT, where
   you set mIPv6Checked to PR_FALSE.  The values of
   mIPv6Checked and mIPv6ServerAddress need to be kept
   consistent.

Comment 12

[John G. Myers]

Attachment: Revised patch

Comment 13

[John G. Myers]

In S_pasv(), changed an assignment of mIPv6ServerAddress to an assertion.

In StopProcessing(), added an assignment of mIPv6Checked to PR_FALSE.  We want 
to free the mIPv6ServerAddress here because the mCPipe it is associated with is 
being released.

We don't need to free mIPv6ServerAddress in FTP_COMMAND_CONNECT as it should 
always be null here.  If it isn't null here, that will get caught by the 
assertion in S_pasv().  We could probably replace the assignment of mIPv6Checked 
here with an assertion.

Comment 14

[Wan-Teh Chang]

In StopProcessing(), since mCPipe is being released, how about
moving the assignment 'mIPv6Checked = PR_FALSE' to the outside
of the if statement?
     mIPv6Checked = PR_FALSE;
     if (mIPv6ServerAddress) {
         nsAllocator::Free(mIPv6ServerAddress);
         mIPv6ServerAddress = 0;
     }

Comment 15

[John G. Myers]

D'oh!  You're right.

Comment 16

[John G. Myers]

Attachment: Revised patch

Comment 17

[Wan-Teh Chang]

I reviewed patch id=8534.  I found one
problem that I didn't notice before.

In S_pasv(), it would be more robust to also
check the address family:
    if (PR_StringToNetAddr(mIPv6ServerAddress, &addr) != PR_SUCCESS ||
        addr.raw.family != PR_AF_INET6 ||
        PR_IsNetAddrType(&addr, PR_IpAddrV4Mapped)) {
        ...
    }

Two other suggestions:
1. In FTP_COMMAND_CONNECT, the assignment of mIPv6Checked
   to PR_FALSE could be replaced by an assertion:
       PR_ASSERT(mIPv6Checked == PR_FALSE);
2. In S_pasv(), the PR_LOG statement prints "addr is null"
   if the server is IPv4 (the common case), which could be
   confusing to someone unfamiliar with this code (because
   null usually indicates some kind of error).

Comment 18

[John G. Myers]

PR_IsNetAddrType() is defined as checking the address family, so checking the 
address family in the caller would be redundant, would it not?

The PR_LOG() statement is for debugging.  The only people enabling the logging 
should be people who know something about the code.  This could be changed to 
"addr is v4".

These last review items are a matter of personal preference.  I would appreciate 
if valeski would review the patch and give his opinion.

Comment 19

[Wan-Teh Chang]

PR_IsNetAddrType(PR_IpAddrV4Mapped) returns PR_TRUE
if the address is an IPv4-mapped *IPv6 address*.
It returns PR_FALSE if the address is a plain IPv4
address.  This is why I suggested checking the
address family so that the code does not rely on the
fact that Necko uses IPv6 addresses exclusively.

Comment 20

[John G. Myers]

Necko can be relied on to use IPv6 addresses exclusively.

Comment 21

[John G. Myers]

Perhaps we should add an:

PR_ASSERT(addr.raw.family == PR_AF_INET6);

statement?

Comment 22

[Wan-Teh Chang]

Yes, an assertion would be good.

Comment 23

[John G. Myers]

Attachment: Revised patch

Comment 24

[Wan-Teh Chang]

I reviewed the latest patch (id=8560).
Just need to change "v6" to "v4" in the
PR_LOG statement.  Thanks for making all
the changes.

Judson, are you planning to check this
in before 5/16?

Comment 25

[Judson Valeski]

patch has been applied

Comment 26

[benc]

Relieving tever of IPv6 related QA.

Comment 27

[benc]

-> off netscape qa radar.