Two minor fixes for nsFtpConnectionThread.cpp

VERIFIED FIXED in M16

Status

()

P3
normal
VERIFIED FIXED
19 years ago
19 years ago

People

(Reporter: wtc, Assigned: gagan)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

19 years ago
I'm going to attach a patch that contains two fixes
for mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp.

1. In nsFtpConnectionThread::R_pasv(), we have
    PRInt32 h0, h1, h2, h3, p0, p1;
    ...
    PRInt32 fields = PR_sscanf(ptr,
#ifdef __alpha
        "%d,%d,%d,%d,%d,%d",
#else
        "%ld,%ld,%ld,%ld,%ld,%ld",
#endif
        &h0, &h1, &h2, &h3, &p0, &p1);

  The #ifdef __alpha is not needed.  The %ld format
  for PR_snprintf and PR_sscanf specifies PRInt32, not
  long.  I checked our documentation
  (http://www.mozilla.org/docs/refList/refNSPR/prprf.html)
  and found that this is not clearly documented.
      %hd   PRInt16
      %d    PRIntn
      %ld   PRInt32
      %lld  PRInt64

2. In nsFtpConnectionThread::R_pasv(), the while loop
  to find the beginning of the returned address string
  is not robust against a malformed response.
    // The returned address string can be of the form
    // (xxx,xxx,xxx,xxx,ppp,ppp) or
    //  xxx,xxx,xxx,xxx,ppp,ppp (without parens)
    ptr = response;
    while (*ptr++) {
        if (*ptr == '(') {
            // move just beyond the open paren
            ptr++;
            break;
        }
        if (*ptr == ',') {
            ptr--;
            // backup to the start of the digits
            while ( (*ptr >= '0') && (*ptr <= '9'))
                ptr--;
            ptr++; // get back onto the numbers
            break;
        }
    } // end while

  The ++ in 'while (*ptr++)' will move 'ptr' beyond
  the terminating null byte if 'response' does not
  contain any '(' and ','.

  The inner while loop, which moves 'ptr' backward,
  needs to check for (ptr >= response) to prevent
  'ptr' from moving past the beginning of 'response'.
  A problematic malformed 'response' for this would
  be "9,9".

Copied jgmyers because this is in the area that his
IPv6 FTP patch touches (bug #28874).
(Reporter)

Comment 1

19 years ago
Created attachment 8341 [details] [diff] [review]
Proposed patch.

Comment 2

19 years ago
The conflict between this patch and bug #28874 is trivial to resolve--it's just 
a difference of indentation.
(Assignee)

Comment 3

19 years ago
will check this in tonite. thx wtc.
Status: NEW → ASSIGNED
Target Milestone: --- → M16
(Assignee)

Comment 4

19 years ago
checked in. 
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED
(Reporter)

Comment 5

19 years ago
I verified the fix.  Thanks Gagan.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.