ftp: store dir components as start+len instead of memdup'ing

- Avoids allocating every path segment separately
- Improved directory handling in connection reuse

Closes #18312

diff --git a/lib/ftp.c b/lib/ftp.c
index b7ab845d4e653510ce0ba708a9bf297c063e28b1..1da756b9dcaf34797e72a365b001243998ed5d4a 100644 (file)
--- a/lib/ftp.c
+++ b/lib/ftp.c
@@ -331,15 +331,10 @@ static void close_secondarysocket(struct Curl_easy *data,
 
 static void freedirs(struct ftp_conn *ftpc)
 {
-  if(ftpc->dirs) {
-    int i;
-    for(i = 0; i < ftpc->dirdepth; i++)
-      free(ftpc->dirs[i]);
-    Curl_safefree(ftpc->dirs);
-    ftpc->dirdepth = 0;
-  }
+  Curl_safefree(ftpc->dirs);
+  ftpc->dirdepth = 0;
   Curl_safefree(ftpc->rawpath);
-  Curl_safefree(ftpc->file);
+  ftpc->file = NULL;
   Curl_safefree(ftpc->newhost);
 }
 
@@ -813,6 +808,20 @@ static CURLcode ftp_domore_pollset(struct Curl_easy *data,
   return Curl_pp_pollset(data, &ftpc->pp, ps);
 }
 
+static int pathlen(struct ftp_conn *ftpc, int num)
+{
+  DEBUGASSERT(ftpc->dirs);
+  DEBUGASSERT(ftpc->dirdepth > num);
+  return ftpc->dirs[num].len;
+}
+
+static const char *pathpiece(struct ftp_conn *ftpc, int num)
+{
+  DEBUGASSERT(ftpc->dirs);
+  DEBUGASSERT(ftpc->dirdepth > num);
+  return &ftpc->rawpath[ ftpc->dirs[num].start ];
+}
+
 /* This is called after the FTP_QUOTE state is passed.
 
    ftp_state_cwd() sends the range of CWD commands to the server to change to
@@ -831,13 +840,13 @@ static CURLcode ftp_state_cwd(struct Curl_easy *data,
   else {
     /* FTPFILE_NOCWD with full path: expect ftpc->cwddone! */
     DEBUGASSERT((data->set.ftp_filemethod != FTPFILE_NOCWD) ||
-                !(ftpc->dirdepth && ftpc->dirs[0][0] == '/'));
+                !(ftpc->dirdepth && ftpc->rawpath[0] == '/'));
 
     ftpc->count2 = 0; /* count2 counts failed CWDs */
 
     if(data->conn->bits.reuse && ftpc->entrypath &&
        /* no need to go to entrypath when we have an absolute path */
-       !(ftpc->dirdepth && ftpc->dirs[0][0] == '/')) {
+       !(ftpc->dirdepth && ftpc->rawpath[0] == '/')) {
       /* This is a reused connection. Since we change directory to where the
          transfer is taking place, we must first get back to the original dir
          where we ended up after login: */
@@ -852,8 +861,8 @@ static CURLcode ftp_state_cwd(struct Curl_easy *data,
         ftpc->cwdcount = 1;
         /* issue the first CWD, the rest is sent when the CWD responses are
            received... */
-        result = Curl_pp_sendf(data, &ftpc->pp, "CWD %s",
-                               ftpc->dirs[ftpc->cwdcount -1]);
+        result = Curl_pp_sendf(data, &ftpc->pp, "CWD %.*s",
+                               pathlen(ftpc, 0), pathpiece(ftpc, 0));
         if(!result)
           ftp_state(data, ftpc, FTP_CWD);
       }
@@ -3055,8 +3064,9 @@ static CURLcode ftp_pp_statemachine(struct Curl_easy *data,
           create the dir) this then allows for a second try to CWD to it. */
           ftpc->count3 = (data->set.ftp_create_missing_dirs == 2) ? 1 : 0;
 
-          result = Curl_pp_sendf(data, &ftpc->pp, "MKD %s",
-                                 ftpc->dirs[ftpc->cwdcount - 1]);
+          result = Curl_pp_sendf(data, &ftpc->pp, "MKD %.*s",
+                                 pathlen(ftpc, ftpc->cwdcount - 1),
+                                 pathpiece(ftpc, ftpc->cwdcount - 1));
           if(!result)
             ftp_state(data, ftpc, FTP_MKD);
         }
@@ -3071,12 +3081,15 @@ static CURLcode ftp_pp_statemachine(struct Curl_easy *data,
       else {
         /* success */
         ftpc->count2 = 0;
-        if(++ftpc->cwdcount <= ftpc->dirdepth)
-          /* send next CWD */
-          result = Curl_pp_sendf(data, &ftpc->pp, "CWD %s",
-                                 ftpc->dirs[ftpc->cwdcount - 1]);
-        else
+        if(ftpc->cwdcount >= ftpc->dirdepth)
           result = ftp_state_mdtm(data, ftpc, ftp);
+        else {
+          ftpc->cwdcount++;
+          /* send next CWD */
+          result = Curl_pp_sendf(data, &ftpc->pp, "CWD %.*s",
+                                 pathlen(ftpc, ftpc->cwdcount - 1),
+                                 pathpiece(ftpc, ftpc->cwdcount - 1));
+        }
       }
       break;
 
@@ -3089,8 +3102,9 @@ static CURLcode ftp_pp_statemachine(struct Curl_easy *data,
       else {
         ftp_state(data, ftpc, FTP_CWD);
         /* send CWD */
-        result = Curl_pp_sendf(data, &ftpc->pp, "CWD %s",
-                               ftpc->dirs[ftpc->cwdcount - 1]);
+        result = Curl_pp_sendf(data, &ftpc->pp, "CWD %.*s",
+                               pathlen(ftpc, ftpc->cwdcount - 1),
+                               pathpiece(ftpc, ftpc->cwdcount - 1));
       }
       break;
 
@@ -3258,7 +3272,6 @@ static CURLcode ftp_done(struct Curl_easy *data, CURLcode status,
   ssize_t nread;
   int ftpcode;
   CURLcode result = CURLE_OK;
-  const char *rawPath = NULL;
 
   if(!ftp || !ftpc)
     return CURLE_OK;
@@ -3301,6 +3314,7 @@ static CURLcode ftp_done(struct Curl_easy *data, CURLcode status,
       Curl_set_in_callback(data, TRUE);
       data->set.chunk_end(data->set.wildcardptr);
       Curl_set_in_callback(data, FALSE);
+      freedirs(ftpc);
     }
     ftpc->known_filesize = -1;
   }
@@ -3314,33 +3328,31 @@ static CURLcode ftp_done(struct Curl_easy *data, CURLcode status,
     ftpc->prevpath = NULL; /* no path remembering */
   }
   else { /* remember working directory for connection reuse */
-    rawPath = ftpc->rawpath;
-    if((data->set.ftp_filemethod == FTPFILE_NOCWD) && (rawPath[0] == '/'))
-      ; /* full path => no CWDs happened => keep ftpc->prevpath */
-    else {
-      size_t pathLen = strlen(ftpc->rawpath);
+    const char *rawPath = ftpc->rawpath;
+    if(rawPath) {
+      if((data->set.ftp_filemethod == FTPFILE_NOCWD) && (rawPath[0] == '/'))
+        ; /* full path => no CWDs happened => keep ftpc->prevpath */
+      else {
+        size_t pathLen = strlen(ftpc->rawpath);
 
-      free(ftpc->prevpath);
+        free(ftpc->prevpath);
 
-      if(!ftpc->cwdfail) {
-        if(data->set.ftp_filemethod == FTPFILE_NOCWD)
-          pathLen = 0; /* relative path => working directory is FTP home */
+        if(!ftpc->cwdfail) {
+          if(data->set.ftp_filemethod == FTPFILE_NOCWD)
+            pathLen = 0; /* relative path => working directory is FTP home */
+          else
+            /* file is url-decoded */
+            pathLen -= ftpc->file ? strlen(ftpc->file) : 0;
+          ftpc->prevpath = Curl_memdup0(rawPath, pathLen);
+        }
         else
-          /* file is url-decoded */
-          pathLen -= ftpc->file ? strlen(ftpc->file) : 0;
-        ftpc->prevpath = Curl_memdup0(rawPath, pathLen);
+          ftpc->prevpath = NULL; /* no path */
       }
-      else
-        ftpc->prevpath = NULL; /* no path */
     }
-
     if(ftpc->prevpath)
       infof(data, "Remembering we are in dir \"%s\"", ftpc->prevpath);
   }
 
-  /* free the dir tree and file parts */
-  freedirs(ftpc);
-
   /* shut down the socket to inform the server we are done */
 
 #ifdef UNDER_CE
@@ -4158,6 +4170,20 @@ static CURLcode ftp_disconnect(struct Curl_easy *data,
   return CURLE_OK;
 }
 
+static size_t numof_slashes(const char *str)
+{
+  const char *slashPos;
+  size_t num = 0;
+  do {
+    slashPos = strchr(str, '/');
+    if(slashPos) {
+      ++num;
+      str = slashPos + 1;
+    }
+  } while(slashPos);
+  return num;
+}
+
 /***********************************************************************
  *
  * ftp_parse_url_path()
@@ -4179,6 +4205,8 @@ CURLcode ftp_parse_url_path(struct Curl_easy *data,
   ftpc->ctl_valid = FALSE;
   ftpc->cwdfail = FALSE;
 
+  if(ftpc->rawpath)
+    freedirs(ftpc);
   /* url-decode ftp path before further evaluation */
   result = Curl_urldecode(ftp->path, 0, &ftpc->rawpath, &pathLen, REJECT_CTRL);
   if(result) {
@@ -4209,15 +4237,11 @@ CURLcode ftp_parse_url_path(struct Curl_easy *data,
           dirlen = 1;
 
         ftpc->dirs = calloc(1, sizeof(ftpc->dirs[0]));
-        if(!ftpc->dirs) {
+        if(!ftpc->dirs)
           return CURLE_OUT_OF_MEMORY;
-        }
-
-        ftpc->dirs[0] = Curl_memdup0(rawPath, dirlen);
-        if(!ftpc->dirs[0]) {
-          return CURLE_OUT_OF_MEMORY;
-        }
 
+        ftpc->dirs[0].start = 0;
+        ftpc->dirs[0].len = (int)dirlen;
         ftpc->dirdepth = 1; /* we consider it to be a single dir */
         fileName = slashPos + 1; /* rest is filename */
       }
@@ -4230,49 +4254,45 @@ CURLcode ftp_parse_url_path(struct Curl_easy *data,
       /* current position: begin of next path component */
       const char *curPos = rawPath;
 
-      /* number of entries allocated for the 'dirs' array */
-      size_t dirAlloc = 0;
-      const char *str = rawPath;
-      for(; *str != 0; ++str)
-        if(*str == '/')
-          ++dirAlloc;
+      /* number of entries to allocate for the 'dirs' array */
+      size_t dirAlloc = numof_slashes(rawPath);
+
+      if(dirAlloc >= 1000)
+        /* suspiciously deep dir hierarchy */
+        return CURLE_URL_MALFORMAT;
 
       if(dirAlloc) {
         ftpc->dirs = calloc(dirAlloc, sizeof(ftpc->dirs[0]));
-        if(!ftpc->dirs) {
+        if(!ftpc->dirs)
           return CURLE_OUT_OF_MEMORY;
-        }
 
         /* parse the URL path into separate path components */
-        /* !checksrc! disable EQUALSNULL 1 */
-        while((slashPos = strchr(curPos, '/')) != NULL) {
-          size_t compLen = slashPos - curPos;
+        while(dirAlloc--) {
+          const char *spos = strchr(curPos, '/');
+          size_t clen = spos - curPos;
 
           /* path starts with a slash: add that as a directory */
-          if((compLen == 0) && (ftpc->dirdepth == 0))
-            ++compLen;
+          if(!clen && (ftpc->dirdepth == 0))
+            ++clen;
 
           /* we skip empty path components, like "x//y" since the FTP command
              CWD requires a parameter and a non-existent parameter a) does not
              work on many servers and b) has no effect on the others. */
-          if(compLen > 0) {
-            char *comp = Curl_memdup0(curPos, compLen);
-            if(!comp) {
-              return CURLE_OUT_OF_MEMORY;
-            }
-            ftpc->dirs[ftpc->dirdepth++] = comp;
+          if(clen) {
+            ftpc->dirs[ftpc->dirdepth].start = (int)(curPos - rawPath);
+            ftpc->dirs[ftpc->dirdepth].len = (int)clen;
+            ftpc->dirdepth++;
           }
-          curPos = slashPos + 1;
+          curPos = spos + 1;
         }
       }
-      DEBUGASSERT((size_t)ftpc->dirdepth <= dirAlloc);
       fileName = curPos; /* the rest is the filename (or empty) */
     }
     break;
   } /* switch */
 
   if(fileName && *fileName)
-    ftpc->file = strdup(fileName);
+    ftpc->file = fileName;
   else
     ftpc->file = NULL; /* instead of point to a zero byte,
                             we make it a NULL pointer */

diff --git a/lib/ftp.h b/lib/ftp.h
index 4560630daa3a2413ab24352f859248122127e6cc..aba1db7f2d3d6f96e503d25c271d9b2d9e878d90 100644 (file)
--- a/lib/ftp.h
+++ b/lib/ftp.h
@@ -120,6 +120,11 @@ struct FTP {
   curl_off_t downloadsize;
 };
 
+/* one struct entry for each path component (of 'rawpath') */
+struct pathcomp {
+  int start; /* start column */
+  int len;   /* length in bytes */
+};
 
 /* ftp_conn is used for struct connection-oriented data in the connectdata
    struct */
@@ -128,9 +133,9 @@ struct ftp_conn {
   char *account;
   char *alternative_to_user;
   char *entrypath; /* the PWD reply when we logged on */
-  char *file;    /* url-decoded filename (or path) */
+  const char *file; /* url-decoded filename (or path), points into rawpath */
   char *rawpath; /* URL decoded, allocated, version of the path */
-  char **dirs;   /* realloc()ed array for path components */
+  struct pathcomp *dirs; /* allocated array for path components */
   char *newhost; /* the (allocated) IP addr or hostname to connect the data
                     connection to */
   char *prevpath;   /* url-decoded conn->path from the previous transfer */

diff --git a/tests/data/test1113 b/tests/data/test1113
index c0e1230c3562b1983f395885446a41812e012cfb..c7eaf2e9382ca10aae2d9ee10a812138dbbc0735 100644 (file)
--- a/tests/data/test1113
+++ b/tests/data/test1113
@@ -65,9 +65,6 @@ EPSV
 RETR file.txt\r
 EPSV\r
 RETR someothertext.txt\r
-CWD /\r
-CWD fully_simulated\r
-CWD DOS\r
 EPSV\r
 TYPE A\r
 LIST\r

diff --git a/tests/data/test142 b/tests/data/test142
index db17e3074e6ab798c9bc805a0d82ad8ee90e8edb..e09b744f938081b3979093385ecefa6612ffe439 100644 (file)
--- a/tests/data/test142
+++ b/tests/data/test142
@@ -189,5 +189,8 @@ SIZE %TESTNUMBER
 RETR %TESTNUMBER\r
 QUIT\r
 </protocol>
+<limits>
+Allocations: 170
+</limits>
 </verify>
 </testcase>

diff --git a/tests/data/test574 b/tests/data/test574
index 50d2120f04d35403a3b1ff058b3f0e1c04ab72d7..46c4883b6100e02d154cf0468d3e8bd77b2a3bf6 100644 (file)
--- a/tests/data/test574
+++ b/tests/data/test574
@@ -62,9 +62,6 @@ EPSV
 RETR file.txt\r
 EPSV\r
 RETR someothertext.txt\r
-CWD /\r
-CWD fully_simulated\r
-CWD UNIX\r
 EPSV\r
 TYPE A\r
 LIST\r

diff --git a/tests/data/test575 b/tests/data/test575
index fbb7343d96705f8c7db6a35a9e6b6f18d8346c20..4ed302fbb1ba21322e3d143ddc73fef06ab3db83 100644 (file)
--- a/tests/data/test575
+++ b/tests/data/test575
@@ -60,9 +60,6 @@ EPSV
 RETR file.txt\r
 EPSV\r
 RETR someothertext.txt\r
-CWD /\r
-CWD fully_simulated\r
-CWD UNIX\r
 EPSV\r
 TYPE A\r
 LIST\r