[bug-inetutils] Bug in inetutils-1.6: FTPD transfers files with a size of 0

[Rachid Koucha]

Hi,

On platforms where HAVE_MMAP is not defined, FTPD transfers file with a size of 0.

Here is an analysis that I have done for my company (Alcatel-Lucent).


The RETR command sent by the client to the server to get a file, triggers the following:

• ftpcmd.y (yacc grammar) triggers the retrieve() function with the following rule:

    | RETR check_login SP pathname CRLF
        {
            if ($2 && $4 != NULL)
                retrieve((char *) 0, $4);
            if ($4 != NULL)
                free($4);

• ftpd.c/retrieve(NULL, pathname) begins with the following:

size_t buffer_size = 0;

if (cmd == 0)
  {
    fin = fopen (name, "r"), closefunc = fclose;
    st.st_size = 0;
  }

• ...the function gets the size of the file to transfer (st.st_size):

  if (cmd == 0 && (fstat (fileno (fin), &st) < 0 || !S_ISREG (st.st_mode)))
    {
      reply (550, "%s: not a plain file.", name);
      goto done;
    }

• ...the function calls datacon() to establish the data connection to transfer the data of the file:

  dout = dataconn (name, st.st_size, "w");

• ... then it calls  send_data() to send the content of the file over the data connection:

  send_data (fin, dout, buffer_size);

• But as you can see, the variable "buffer_size" passed to send_data() is equal to 0 because it has not been modified since the beginning of the function where it has been initialized to 0
• In ftpd.c/send_data(), the parameter "blksize" is equal to 0 (value of the preceding "buffer_size" variable)
  
• ftpd.c/send_data() behaves in two possible ways. Either it reads the file to send with mmap() or it reads the file with the file system services. This is controlled by HAVE_MMAP flag. This flag is defined with the "configure" script located at the top level of the inetutils subtree. When "configure" is run, it generates the file "config.h" at the top level of the inetutils subtree. On my laptop running Linux ubuntu, "configure" sets the following in config.h:

/* Define to 1 if you have a working `mmap' system call. */
#define HAVE_MMAP 1

• When it is defined, everything is fine in send_data() both for ASCII mode (case TYPE_A:) and Binary mode (case TYPE_I:) because we don't use the parameter "blksize". For example, for TYPE I, we run the following code to send the data:

    case TYPE_I:
    case TYPE_L:
#ifdef HAVE_MMAP
      if (file_size > 0 && curpos >= 0 && buf != MAP_FAILED)
    {
      bp = buf;
      len = filesize;
      do
        {
          cnt = write (netfd, bp, len);
          len -= cnt;
          bp += cnt;
          if (cnt > 0)
        byte_count += cnt;
        }
      while (cnt > 0 && len > 0);
      transflag = 0;
      munmap (buf, (size_t) filesize);
      if (cnt < 0)
        goto data_err;
      reply (226, "Transfer complete with mmap (blksize = %u).", (u_int)blksize);
      return;
    }
#endif

• But if HAVE_MMAP is not defined in "config.h" (I commented it out), the "case TYPE_A" still works because it does not use "blksize", but the "case TYPE I" does not work because it uses the parameter "blksize" which has a size of 0. The following while loop stops immediately as we call read() with a blksize equal to 0:
  

      buf = malloc ((u_int) blksize);
      if (buf == NULL)
    {
      transflag = 0;
      perror_reply (451, "Local resource failure: malloc");
      return;
    }
      while ((cnt = read (filefd, buf, (u_int) blksize)) > 0 &&
         write (netfd, buf, cnt) == cnt)
    byte_count += cnt;
      transflag = 0;
      free (buf);
      if (cnt != 0)
    {
      if (cnt < 0)
        goto file_err;
      goto data_err;
    }
      reply (226, "Transfer complete.");
      return;

So, I would propose the following patch:

• In ftpd.c/retrieve(), we initialize the local_variable "buffer_size" to 4096 bytes or better ST_BLKSIZE (st) provided that we call fstat(fileno (fin), &st):

void
retrieve (const char *cmd, const char *name)
{
  FILE *fin, *dout;
  struct stat st;
  int (*closefunc) (FILE *);
  size_t buffer_size = 0;

  if (cmd == 0)
    {
      fin = fopen (name, "r"), closefunc = fclose;
      st.st_size = 0;
      buffer_size = 4096;
    }
  else
    {
      char line[BUFSIZ];

      snprintf (line, sizeof line, cmd, name);
      name = line;
      fin = ftpd_popen (line, "r"), closefunc = ftpd_pclose;
      st.st_size = -1;
      buffer_size = BUFSIZ;
    }

I tried it with HAVE_MMAP not defined in "config.h" and it works.


Regards.

Rachid Koucha.