Bug 1976: Failure when retrieving a directory listing from an FTP server when the data are transmitted in huge amount of tiny chunks

https://winscp.net/tracker/1976

Source commit: 83a66d42769a20364935856660f495a021bdd483

source/filezilla/FtpControlSocket.cpp

@@ -1736,8 +1736,6 @@ void CFtpControlSocket::List(BOOL bFinish, int nError /*=FALSE*/, CServerPath pa
 
     int num = 0;
     pData->pDirectoryListing = new t_directory;
-    if (GetOptionVal(OPTION_DEBUGSHOWLISTING))
-      m_pTransferSocket->m_pListResult->SendToMessageLog();
     pData->pDirectoryListing->direntry = m_pTransferSocket->m_pListResult->getList(num);
     pData->pDirectoryListing->num = num;
     if (m_pTransferSocket->m_pListResult->m_server.nServerType & FZ_SERVERTYPE_SUB_FTP_VMS && m_CurrentServer.nServerType & FZ_SERVERTYPE_FTP)
@@ -2416,14 +2414,10 @@ void CFtpControlSocket::ListFile(CString filename, const CServerPath &path)
     else
     {
       USES_CONVERSION;
-      int size = m_ListFile.GetLength();
-      char *buffer = new char[size + 1];
-      memmove(buffer, (LPCSTR)m_ListFile, m_ListFile.GetLength());
+      CStringA Buf = m_ListFile + '\n';
       const bool mlst = true;
       CFtpListResult * pListResult = CreateListResult(mlst);
-      pListResult->AddData(buffer, size);
-      if (GetOptionVal(OPTION_DEBUGSHOWLISTING))
-        pListResult->SendToMessageLog();
+      pListResult->AddData(static_cast<const char *>(Buf), Buf.GetLength());
       pData->direntry = pListResult->getList(num);
       if (pListResult->m_server.nServerType & FZ_SERVERTYPE_SUB_FTP_VMS && m_CurrentServer.nServerType & FZ_SERVERTYPE_FTP)
         m_CurrentServer.nServerType |= FZ_SERVERTYPE_SUB_FTP_VMS;
@@ -2793,8 +2787,6 @@ void CFtpControlSocket::FileTransfer(t_transferfile *transferfile/*=0*/,BOOL bFi
 
       int num=0;
       pData->pDirectoryListing=new t_directory;
-      if (GetOptionVal(OPTION_DEBUGSHOWLISTING))
-        m_pTransferSocket->m_pListResult->SendToMessageLog();
       pData->pDirectoryListing->direntry=m_pTransferSocket->m_pListResult->getList(num);
       pData->pDirectoryListing->num=num;
       if (m_pTransferSocket->m_pListResult->m_server.nServerType&FZ_SERVERTYPE_SUB_FTP_VMS && m_CurrentServer.nServerType&FZ_SERVERTYPE_FTP)
@@ -6364,7 +6356,9 @@ bool CFtpControlSocket::CheckForcePasvIp(CString & host)
 
 CFtpListResult * CFtpControlSocket::CreateListResult(bool mlst)
 {
-  CFtpListResult * Result = new CFtpListResult(m_CurrentServer, mlst, &m_bUTF8, GetOptionVal(OPTION_VMSALLREVISIONS));
+  CFtpListResult * Result =
+    new CFtpListResult(
+      m_CurrentServer, mlst, &m_bUTF8, GetOptionVal(OPTION_VMSALLREVISIONS), GetOptionVal(OPTION_DEBUGSHOWLISTING));
   Result->InitIntern(GetIntern());
   return Result;
 }

source/filezilla/FtpListResult.cpp

@@ -4,20 +4,13 @@
 #include "FileZillaApi.h"
 #include <WideStrUtils.hpp>
 
-CFtpListResult::CFtpListResult(t_server server, bool mlst, bool *bUTF8, bool vmsAllRevisions)
+CFtpListResult::CFtpListResult(t_server server, bool mlst, bool *bUTF8, bool vmsAllRevisions, bool debugShowListing)
 {
-  listhead=curpos=0;
-
   m_mlst = mlst;
   m_server = server;
   m_bUTF8 = bUTF8;
   m_vmsAllRevisions = vmsAllRevisions;
-
-  pos=0;
-
-  m_prevline=0;
-  m_curline=0;
-  m_curlistaddpos=0;
+  m_debugShowListing = debugShowListing;
 
   //Fill the month names map
 
@@ -234,115 +227,32 @@ CFtpListResult::CFtpListResult(t_server server, bool mlst, bool *bUTF8, bool vms
   m_MonthNamesMap[L"avg"] = 8;
 }
 
-
-CFtpListResult::~CFtpListResult()
-{
-  t_list *ptr=listhead;
-  t_list *ptr2;
-  while (ptr)
-  {
-    delete [] ptr->buffer;
-    ptr2=ptr;
-    ptr=ptr->next;
-    delete ptr2;
-  }
-  if (m_prevline)
-    delete [] m_prevline;
-  if (m_curline)
-    delete [] m_curline;
-}
-
-void CFtpListResult::DoParseLine(char *& Line, t_directory::t_direntry & DirEntry)
+t_directory::t_direntry * CFtpListResult::getList(int & Num)
 {
-  int ServerType;
-  char * TmpLine = new char[strlen(Line) + 1];
-  strcpy(TmpLine, Line);
-  bool Result = parseLine(TmpLine, strlen(TmpLine), DirEntry, ServerType);
-  delete [] TmpLine;
-  if (Result)
+  if (!FBuffer.IsEmpty())
   {
-    if (ServerType != 0)
-    {
-      m_server.nServerType |= ServerType;
-    }
-    if ((DirEntry.name != L".") && (DirEntry.name != L".."))
-    {
-      AddLine(DirEntry);
-    }
-    if (m_prevline != NULL)
-    {
-      delete [] m_prevline;
-      m_prevline = NULL;
-    }
-    if (m_curline != Line)
-    {
-      delete [] m_curline;
-    }
-    delete [] Line;
-    Line = GetLine();
-    m_curline = Line;
+    SendLineToMessageLog("Unparsed listing:");
+    SendLineToMessageLog(FBuffer);
+  }
+  Num = m_EntryList.size();
+  t_directory::t_direntry * Result;
+  if (Num == 0)
+  {
+    SendLineToMessageLog("<Empty directory listing>");
+    Result = NULL;
   }
   else
   {
-    if (m_prevline != NULL)
+    Result = new t_directory::t_direntry[Num];
+    int I = 0;
+    for (tEntryList::iterator Iter = m_EntryList.begin(); Iter != m_EntryList.end(); Iter++, I++)
     {
-      if (m_curline != Line)
-      {
-        delete [] m_prevline;
-        m_prevline = m_curline;
-        delete [] Line;
-        Line = GetLine();
-        m_curline = Line;
-      }
-      else
-      {
-        Line = new char[strlen(m_prevline) + strlen(m_curline) + 2];
-        sprintf(Line, "%s %s", m_prevline, m_curline);
-      }
-    }
-    else
-    {
-      m_prevline = Line;
-      Line = GetLine();
-      m_curline = Line;
+      Result[I] = *Iter;
     }
+    m_EntryList.clear();
   }
-}
 
-t_directory::t_direntry *CFtpListResult::getList(int &num)
-{
-  #ifdef _DEBUG
-  USES_CONVERSION;
-  #endif
-  char *line=GetLine();
-  m_curline=line;
-  while (line)
-  {
-    t_directory::t_direntry direntry;
-    DoParseLine(line, direntry);
-  }
-  if (m_prevline)
-  {
-    delete [] m_prevline;
-    m_prevline=0;
-  }
-  if (m_curline!=line)
-    delete [] m_curline;
-  delete [] line;
-  m_curline=0;
-
-  num=m_EntryList.size();
-  if (!num)
-    return 0;
-  t_directory::t_direntry *res=new t_directory::t_direntry[num];
-  int i=0;
-  for (tEntryList::iterator iter=m_EntryList.begin();iter!=m_EntryList.end();iter++, i++)
-  {
-    res[i]=*iter;
-  }
-  m_EntryList.clear();
-
-  return res;
+  return Result;
 }
 
 BOOL CFtpListResult::parseLine(const char *lineToParse, const int linelen, t_directory::t_direntry &direntry, int &nFTPServerType)
@@ -404,159 +314,111 @@ BOOL CFtpListResult::parseLine(const char *lineToParse, const int linelen, t_dir
   return FALSE;
 }
 
-void CFtpListResult::AddData(char *data, int size)
+bool CFtpListResult::IsNewLineChar(char C) const
 {
-  #ifdef _DEBUG
-  USES_CONVERSION;
-  #endif
-  if (!size)
-    return;
+  return (C == '\r') || (C == '\n');
+}
 
-  if (!m_curlistaddpos)
-    m_curlistaddpos = new t_list;
-  else
-  {
-    m_curlistaddpos->next = new t_list;
-    m_curlistaddpos = m_curlistaddpos->next;
-  }
-  if (!listhead)
+void CFtpListResult::AddData(const char * Data, int Size)
+{
+  FBuffer += RawByteString(Data, Size);
+
+  // Just in case the previous buffer was terminated between CR and LF.
+  while (!FBuffer.IsEmpty() && IsNewLineChar(FBuffer[1]))
   {
-    curpos = m_curlistaddpos;
-    listhead = m_curlistaddpos;
+    FBuffer.Delete(1, 1);
   }
-  m_curlistaddpos->buffer = data;
-  m_curlistaddpos->len = size;
-  m_curlistaddpos->next = 0;
 
-  t_list *pOldListPos = curpos;
-  int nOldListBufferPos = pos;
+  bool Found;
+  int Pos;
+  int FirstLineEnd;
+  int Count;
+  bool Restart = true;
 
-  //Try if there are already some complete lines
-  t_directory::t_direntry direntry;
-  char *line = GetLine();
-  m_curline = line;
-  while (line)
+  do
   {
-    if (curpos)
+    if (Restart)
     {
-      pOldListPos = curpos;
-      nOldListBufferPos = pos;
+      Pos = 1;
+      FirstLineEnd = -1;
+      Count = 0;
+      Restart = false;
     }
-    else
+
+    std::vector<RawByteString> Lines;
+    while ((Pos <= FBuffer.Length()) && !IsNewLineChar(FBuffer[Pos]))
     {
-      delete [] line;
-      if (m_curline != line)
-        delete [] m_curline;
-      m_curline = 0;
-      break;
+      Pos++;
     }
-    DoParseLine(line, direntry);
-  }
-  curpos=pOldListPos;
-  pos=nOldListBufferPos;
-
-}
-
-void CFtpListResult::SendToMessageLog()
-{
-  t_list *oldlistpos = curpos;
-  int oldbufferpos = pos;
-  curpos = listhead;
-  pos=0;
-  char *line = GetLine();
-  // Note that FZ_LOG_INFO here is not checked against debug level, as the direct
-  // call to PostMessage bypasses check in LogMessage.
-  // So we get the listing on any logging level, what is actually what we want
-  if (!line)
-  {
-    //Displays a message in the message log
-    t_ffam_statusmessage *pStatus = new t_ffam_statusmessage;
-    pStatus->post = TRUE;
-    pStatus->status = L"<Empty directory listing>";
-    pStatus->type = FZ_LOG_INFO;
-    GetIntern()->PostMessage(FZ_MSG_MAKEMSG(FZ_MSG_STATUS, 0), (LPARAM)pStatus);
-  }
-  while (line)
-  {
-    CString status = line;
-    delete [] line;
-
-    //Displays a message in the message log
-    t_ffam_statusmessage *pStatus = new t_ffam_statusmessage;
-    pStatus->post = TRUE;
-    pStatus->status = status;
-    pStatus->type = FZ_LOG_INFO;
-    if (!GetIntern()->PostMessage(FZ_MSG_MAKEMSG(FZ_MSG_STATUS, 0), (LPARAM)pStatus))
-      delete pStatus;
-
-    line = GetLine();
-  }
-  curpos = oldlistpos;
-  pos = oldbufferpos;
-}
-
-char * CFtpListResult::GetLine()
-{
-  if (!curpos)
-    return 0;
-  int len=curpos->len;
-  while (curpos->buffer[pos]=='\r' || curpos->buffer[pos]=='\n' || curpos->buffer[pos]==' ' || curpos->buffer[pos]=='\t')
-  {
-    pos++;
-    if (pos>=len)
+    Found = (Pos <= FBuffer.Length());
+    if (Found)
     {
-      curpos=curpos->next;
-      if (!curpos)
-        return 0;
-      len=curpos->len;
-      pos=0;
+      Count++;
+      RawByteString Record = FBuffer.SubString(1, Pos - 1);
+      while ((Pos <= FBuffer.Length()) && IsNewLineChar(FBuffer[Pos]))
+      {
+        Pos++;
+      }
+      if (FirstLineEnd < 0)
+      {
+        FirstLineEnd = Pos;
+      }
+      t_directory::t_direntry DirEntry;
+      int ServerType;
+      RawByteString Line = Record;
+      for (int Index = 1; Index <= Line.Length(); Index++)
+      {
+        if (IsNewLineChar(Line[Index]))
+        {
+          Line[Index] = ' ';
+        }
+      }
+      if (parseLine(Line.c_str(), Line.Length(), DirEntry, ServerType))
+      {
+        if (ServerType != 0)
+        {
+          m_server.nServerType |= ServerType;
+        }
+        if ((DirEntry.name != L".") && (DirEntry.name != L".."))
+        {
+          AddLine(DirEntry);
+        }
+        FBuffer.Delete(1, Pos - 1);
+        Restart = true;
+        SendLineToMessageLog(Record);
+      }
+      else
+      {
+        if (Count == 2)
+        {
+          RawByteString FirstLine = FBuffer.SubString(1, FirstLineEnd - 1).TrimRight();
+          SendLineToMessageLog("Cannot parse line:");
+          SendLineToMessageLog(FirstLine);
+          FBuffer.Delete(1, FirstLineEnd - 1);
+          Restart = true;
+        }
+      }
     }
   }
+  while (Found);
+}
 
-  t_list *startptr=curpos;
-  int startpos=pos;
-  int reslen=0;
-
-  while ((curpos->buffer[pos]!='\n')&&(curpos->buffer[pos]!='\r'))
+void CFtpListResult::SendLineToMessageLog(const RawByteString & Line)
+{
+  if (m_debugShowListing)
   {
-    reslen++;
-    pos++;
-    if (pos>=len)
+    t_ffam_statusmessage * Status = new t_ffam_statusmessage;
+    Status->post = TRUE;
+    Status->status = Line.c_str();
+    Status->type = FZ_LOG_INFO;
+    if (!GetIntern()->PostMessage(FZ_MSG_MAKEMSG(FZ_MSG_STATUS, 0), (LPARAM)Status))
     {
-      curpos=curpos->next;
-      if (!curpos)
-        break;
-      len=curpos->len;
-      pos=0;
+      delete Status;
     }
   }
-
-  char *res = new char[reslen+1];
-  res[reslen]=0;
-  int respos=0;
-  while (startptr!=curpos && reslen)
-  {
-    int copylen=startptr->len-startpos;
-    if (copylen>reslen)
-      copylen=reslen;
-    memcpy(&res[respos],&startptr->buffer[startpos], copylen);
-    reslen-=copylen;
-    respos+=startptr->len-startpos;
-    startpos=0;
-    startptr=startptr->next;
-  }
-  if (curpos && reslen)
-  {
-    int copylen=pos-startpos;
-    if (copylen>reslen)
-      copylen=reslen;
-    memcpy(&res[respos], &curpos->buffer[startpos], copylen);
-  }
-
-  return res;
 }
 
-void CFtpListResult::AddLine(t_directory::t_direntry &direntry)
+void CFtpListResult::AddLine(t_directory::t_direntry & direntry)
 {
   if (m_server.nTimeZoneOffset &&
     direntry.date.hasdate && direntry.date.hastime && !direntry.date.utc)
@@ -1167,22 +1029,16 @@ BOOL CFtpListResult::parseAsMlsd(const char *line, const int linelen, t_director
         if ((value.GetLength() > 14) && (value[13] == ':'))
           direntry.linkTarget = value.Mid(14);
       }
+      // ProFTPD up to 1.3.6rc1 and 1.3.5a incorrectly uses "cdir" for the current working directory.
+      // So at least in MLST, where this would be the only entry, we treat it like "dir".
+      // For MLSD, these will be skipped in AddData.
       else if (!value.CompareNoCase(L"cdir"))
       {
-        // ProFTPD up to 1.3.6rc1 and 1.3.5a incorrectly uses "cdir" for the current working directory.
-        // So at least in MLST, where this would be the only entry, we treat it like "dir".
-        if (m_mlst)
-        {
-          direntry.dir = TRUE;
-        }
-        else
-        {
-          return FALSE;
-        }
+        direntry.dir = TRUE;
       }
       else if (!value.CompareNoCase(L"pdir"))
       {
-        return FALSE;
+        direntry.dir = TRUE;
       }
     }
     else if (factname == L"size")

source/filezilla/FtpListResult.h

@@ -31,10 +31,8 @@ class CFtpListResult : public CApiLog
 {
 public:
   t_server m_server;
-  void SendToMessageLog();
-  void AddData(char * data,int size);
-  CFtpListResult(t_server server, bool mlst, bool * bUTF8, bool vmsAllRevisions);
-  virtual ~CFtpListResult();
+  void AddData(const char * data,int size);
+  CFtpListResult(t_server server, bool mlst, bool * bUTF8, bool vmsAllRevisions, bool debugShowListing);
   t_directory::t_direntry * getList(int & num);
 
 private:
@@ -42,7 +40,6 @@ private:
   tEntryList m_EntryList;
 
   BOOL parseLine(const char * lineToParse, const int linelen, t_directory::t_direntry & direntry, int & nFTPServerType);
-  void DoParseLine(char *& Line, t_directory::t_direntry & DirEntry);
 
   BOOL parseAsVMS(const char * line, const int linelen, t_directory::t_direntry & direntry);
   BOOL parseAsEPLF(const char * line, const int linelen, t_directory::t_direntry & direntry);
@@ -64,13 +61,7 @@ private:
 
   bool parseMlsdDateTime(const CString value, t_directory::t_direntry::t_date & date) const;
 
-  int pos;
-  struct t_list
-  {
-    char * buffer;
-    int len;
-    t_list * next;
-  } * listhead, * curpos, * m_curlistaddpos;
+  RawByteString FBuffer;
 
   typedef std::list<int> tTempData;
   tTempData m_TempData;
@@ -79,6 +70,7 @@ private:
   std::map<CString, int> m_MonthNamesMap;
 
   bool m_vmsAllRevisions;
+  bool m_debugShowListing;
 
 protected:
   bool m_mlst;
@@ -88,10 +80,9 @@ protected:
   const char * strnstr(const char * str, int len, const char * c) const;
   _int64 strntoi64(const char * str, int len) const;
   void AddLine(t_directory::t_direntry & direntry);
-  char * GetLine();
   bool IsNumeric(const char * str, int len) const;
-  char * m_prevline;
-  char * m_curline;
+  bool IsNewLineChar(char C) const;
+  void SendLineToMessageLog(const RawByteString & Line);
 };
 //---------------------------------------------------------------------------
 #endif // FtpListResultH

source/filezilla/TransferSocket.cpp

@@ -114,8 +114,8 @@ void CTransferSocket::OnReceive(int nErrorCode)
     if (m_nTransferState == STATE_STARTING)
       OnConnect(0);
 
-    char *buffer = new char[BUFSIZE];
-    int numread = CAsyncSocketEx::Receive(buffer, BUFSIZE);
+    std::vector<char> Buffer(BUFSIZE);
+    int numread = CAsyncSocketEx::Receive(&Buffer[0], Buffer.size());
     if (numread != SOCKET_ERROR && numread)
     {
       m_LastActiveTime = CTime::GetCurrentTime();
@@ -123,35 +123,33 @@ void CTransferSocket::OnReceive(int nErrorCode)
 #ifndef MPEXT_NO_ZLIB
       if (m_useZlib)
       {
-        m_zlibStream.next_in = (Bytef *)buffer;
+        m_zlibStream.next_in = (Bytef *)&Buffer[0];
         m_zlibStream.avail_in = numread;
-        char *out = new char[BUFSIZE];
-        m_zlibStream.next_out = (Bytef *)out;
+        std::unique_ptr<char []> out(new char[BUFSIZE]);
+        m_zlibStream.next_out = (Bytef *)&out[0];
         m_zlibStream.avail_out = BUFSIZE;
         int res = inflate(&m_zlibStream, 0);
         while (res == Z_OK)
         {
-          m_pListResult->AddData(out, BUFSIZE - m_zlibStream.avail_out);
-          out = new char[BUFSIZE];
-          m_zlibStream.next_out = (Bytef *)out;
+          m_pListResult->AddData(&out[0], BUFSIZE - m_zlibStream.avail_out);
+          out.reset(new char[BUFSIZE]);
+          m_zlibStream.next_out = (Bytef *)&out[0];
           m_zlibStream.avail_out = BUFSIZE;
           res = inflate(&m_zlibStream, 0);
         }
-        delete [] buffer;
         if (res == Z_STREAM_END)
-          m_pListResult->AddData(out, BUFSIZE - m_zlibStream.avail_out);
+          m_pListResult->AddData(&out[0], BUFSIZE - m_zlibStream.avail_out);
         else if (res != Z_OK && res != Z_BUF_ERROR)
         {
-          delete [] out;
           CloseAndEnsureSendClose(CSMODE_TRANSFERERROR);
           return;
         }
-        else
-          delete [] out;
       }
       else
 #endif
-        m_pListResult->AddData(buffer, numread);
+      {
+        m_pListResult->AddData(&Buffer[0], numread);
+      }
       m_transferdata.transfersize += numread;
       t_ffam_transferstatus *status = new t_ffam_transferstatus;
       status->bFileTransfer = FALSE;
@@ -159,8 +157,6 @@ void CTransferSocket::OnReceive(int nErrorCode)
       status->bytes = m_transferdata.transfersize;
       GetIntern()->PostMessage(FZ_MSG_MAKEMSG(FZ_MSG_TRANSFERSTATUS, 0), (LPARAM)status);
     }
-    else
-      delete [] buffer;
     if (!numread)
     {
       CloseAndEnsureSendClose(0);