Reason why ftp fails.

[Brian May <bam@snoopy.apana.org.au>]

Why "NLIST *" doesn't work in protected mode. Fix is attached. I am
not sure if this is correct, other then it works! Not tested
yet on Ultrix. ]

Part 1 of problem, from ftp security.c:

int
sec_getc(FILE *F)
{
    if(sec_complete && data_prot) {
        char c;
        if(sec_read(fileno(F), &c, 1) == 0)
            return EOF;
        return c;
    } else
        return getc(F);
}

This assumes that one character can be read on no characters can be
read. Error conditions (eg if sec_read returns -1, this occurs
for EOF, see below) are interpreted
as "1 byte read". This needs to be fixed. It should never
assume that 1 byte is read when an error occurs, otherwise it
will continue reading the non-existant byte forever (like what
I have been experiencing).

FIX PART 1: Change == to <=. I don't particular like this, as errors
are treated as EOF, and data may be lost without any warning. This
fix cannot be used alone, as
it may cause the lost part of the transfer to be discarded (as explained
below).


FIX PART 2, read following. My comments enclosed in [ and ]:

[ working up the stack tree. sec_read calls sec_get_data, which
calls block_read() ]

static int
block_read(int fd, void *buf, size_t len)
{
    unsigned char *p = buf;
    int b;
    while(len) {
        b = read(fd, p, len);
        if(b <= 0)
            return -1;

[ EOF is reported here and returned as -1. Perhaps it should return 0 for EOF? ]

        len -= b;
        p += b;
    }
    return p - (unsigned char*)buf;
}

static int
sec_get_data(int fd, struct buffer *buf, int level)
{
    int len;

    if(block_read(fd, &len, sizeof(len)) < 0)
        return -1;

[ since block_read returned -1, sec_get_data returns -1. Note that the
rest of this routine would become confused if block_read returned 0 ]

    len = ntohl(len);
    buf->data = realloc(buf->data, len);
    if(block_read(fd, buf->data, len) < 0)
        return -1;
    buf->size = (*mech->decode)(app_data, buf->data, len, data_prot);
    buf->index = 0;
    return 0;
}

int
sec_read(int fd, void *data, int length)
{
    size_t len;
    int rx = 0;

    if(sec_complete == 0 || data_prot == 0)
        return read(fd, data, length);

    if(in_buffer.eof_flag){
        in_buffer.eof_flag = 0;
        return 0;
    }

[ this code does not get used ]

    len = buffer_read(&in_buffer, data, length);
    length -= len;
    rx += len;
    data = (char*)data + len;

    while(length){
        if(sec_get_data(fd, &in_buffer, data_prot) < 0)
            return -1;

[ On EOF or error, sec_get_data returns -1 (as explained above). 
THIS IS WRONG!!!! It could result in data loss, as the current
contents of the buffer is discarded. It also means the following
code is never executed. ]

        if(in_buffer.size == 0) {
            if(rx)
                in_buffer.eof_flag = 1;
            return rx;
        }

[ the code above correctly handles EOF and returns the current
contents of thw buffer ]

        len = buffer_read(&in_buffer, data, length);
        length -= len;
        rx += len;
        data = (char*)data + len;
    }
    return rx;
}


-- 
Brian May <bam@snoopy.apana.org.au>
Index: security.c
===================================================================
RCS file: /homes/bam/cvsroot/debian/heimdal/appl/ftp/ftp/security.c,v
retrieving revision 1.1.1.1
diff -c -r1.1.1.1 security.c
*** security.c	1999/07/24 07:33:42	1.1.1.1
--- security.c	1999/08/18 05:29:51
***************
*** 127,133 ****
  {
      if(sec_complete && data_prot) {
  	char c;
! 	if(sec_read(fileno(F), &c, 1) == 0)
  	    return EOF;
  	return c;
      } else
--- 127,133 ----
  {
      if(sec_complete && data_prot) {
  	char c;
! 	if(sec_read(fileno(F), &c, 1) <= 0)
  	    return EOF;
  	return c;
      } else
***************
*** 141,146 ****
--- 141,148 ----
      int b;
      while(len) {
  	b = read(fd, p, len);
+ 	if(b == 0)
+ 	    return 0;
  	if(b <= 0)
  	    return -1;
  	len -= b;
***************
*** 168,180 ****
  sec_get_data(int fd, struct buffer *buf, int level)
  {
      int len;
      
!     if(block_read(fd, &len, sizeof(len)) < 0)
! 	return -1;
      len = ntohl(len);
      buf->data = realloc(buf->data, len);
!     if(block_read(fd, buf->data, len) < 0)
! 	return -1;
      buf->size = (*mech->decode)(app_data, buf->data, len, data_prot);
      buf->index = 0;
      return 0;
--- 170,190 ----
  sec_get_data(int fd, struct buffer *buf, int level)
  {
      int len;
+     int b;
      
!     buf->size = 0;                     /* Set default, in case of EOF */
!     buf->index = 0;
! 
!     b = block_read(fd, &len, sizeof(len));
!     if(b == 0) return 0;
!     if(b < 0) return -1;
      len = ntohl(len);
+ 
      buf->data = realloc(buf->data, len);
!     b = block_read(fd, buf->data, len);
!     if(b == 0) return 0;             /* Should EOF here return error ??? */
!     if(b < 0) return -1;
! 
      buf->size = (*mech->decode)(app_data, buf->data, len, data_prot);
      buf->index = 0;
      return 0;

PGP signature