Try to make the handling of MLSD a little less memory hungry, especially

for wide directories, by using a sub pool for each file entry, rather that using the temporary pool for all of the entries. While working on this, I found some other pools which were not being properly tagged, which made using the pr_pool_debug_memory() function harder to use.
proftpd · Nov 13, 2016 · 25dffc6 · 25dffc6
1 parent 23acf96
commit 25dffc6
Show file tree

Hide file tree

Showing 6 changed files with 173 additions and 9 deletions.
diff --git a/modules/mod_facts.c b/modules/mod_facts.c
@@ -334,14 +334,23 @@ static size_t facts_mlinfo_fmt(struct mlinfo *info, char *buf, size_t bufsz,
  * channel, wherease MLSD's output is sent via a data transfer, much like
  * LIST or NLST.
  */
+static pool *mlinfo_pool = NULL;
 static char *mlinfo_buf = NULL, *mlinfo_bufptr = NULL;
 static size_t mlinfo_bufsz = 0;
 static size_t mlinfo_buflen = 0;
 
 static void facts_mlinfobuf_init(void) {
   if (mlinfo_buf == NULL) {
     mlinfo_bufsz = pr_config_get_server_xfer_bufsz(PR_NETIO_IO_WR);
-    mlinfo_buf = palloc(session.pool, mlinfo_bufsz);
+
+    if (mlinfo_pool != NULL) {
+      destroy_pool(mlinfo_pool);
+    }
+
+    mlinfo_pool = make_sub_pool(session.pool);
+    pr_pool_tag(mlinfo_pool, "Facts MLSD Buffer Pool");
+
+    mlinfo_buf = palloc(mlinfo_pool, mlinfo_bufsz);
     pr_trace_msg("data", 8, "allocated facts buffer of %lu bytes",
       (unsigned long) mlinfo_bufsz);
   }
@@ -1226,7 +1235,7 @@ MODRET facts_mfmt(cmd_rec *cmd) {
     pr_response_add_err(R_550, "%s: %s", cmd->arg, strerror(xerrno));
 
     pr_cmd_set_errno(cmd, xerrno);
-    errno = xerrno; 
+    errno = xerrno;
     return PR_ERROR(cmd);
   }
 
@@ -1508,7 +1517,8 @@ MODRET facts_mlsd(cmd_rec *cmd) {
 
     memset(&info, 0, sizeof(struct mlinfo));
 
-    info.pool = cmd->tmp_pool;
+    info.pool = make_sub_pool(cmd->tmp_pool);
+    pr_pool_tag(info.pool, "MLSD facts pool");
     if (facts_mlinfo_get(&info, rel_path, dent->d_name, flags,
         fake_uid, fake_gid, fake_mode) < 0) {
       pr_log_debug(DEBUG3, MOD_FACTS_VERSION
@@ -1519,10 +1529,13 @@ MODRET facts_mlsd(cmd_rec *cmd) {
     /* As per RFC3659, the directory being listed should not appear as a
      * component in the paths of the directory contents.
      */
-    info.path = pr_fs_encode_path(cmd->tmp_pool, dent->d_name);
+    info.path = pr_fs_encode_path(info.pool, dent->d_name);
 
     facts_mlinfobuf_add(&info, FACTS_MLINFO_FL_APPEND_CRLF);
 
+    destroy_pool(info.pool);
+    info.pool = NULL;
+
     if (XFER_ABORTED) {
       pr_data_abort(0, 0);
       break;

diff --git a/modules/mod_ident.c b/modules/mod_ident.c
@@ -361,6 +361,7 @@ static int ident_sess_init(void) {
   }
 
   tmp_pool = make_sub_pool(session.pool);
+  pr_pool_tag(tmp_pool, "IdentLookup pool");
 
   /* Perform the RFC1413 lookup */
   pr_log_debug(DEBUG6, MOD_IDENT_VERSION ": performing ident lookup");

diff --git a/modules/mod_xfer.c b/modules/mod_xfer.c
@@ -3871,16 +3871,21 @@ static void xfer_sigusr2_ev(const void *event_data, void *user_data) {
         session.curr_cmd_id == PR_CMD_RETR_ID ||
         session.curr_cmd_id == PR_CMD_STOR_ID ||
         session.curr_cmd_id == PR_CMD_STOU_ID) {
-      pool *p = make_sub_pool(session.pool);
-      cmd_rec *cmd = pr_cmd_alloc(p, 1, session.curr_cmd);
+      pool *tmp_pool;
+      cmd_rec *cmd;
+
+      tmp_pool = make_sub_pool(session.pool);
+      pr_pool_tag(tmp_pool, "Data Transfer SIGUSR2 pool");
+
+      cmd = pr_cmd_alloc(tmp_pool, 1, session.curr_cmd);
 
       /* Rescan the config tree for TransferRates, picking up any possible
        * changes.
        */
       pr_log_debug(DEBUG2, "rechecking TransferRates");
       pr_throttle_init(cmd);
 
-      destroy_pool(p);
+      destroy_pool(tmp_pool);
     }
   }
 

diff --git a/src/data.c b/src/data.c
@@ -95,7 +95,7 @@ static void data_new_xfer(char *filename, int direction) {
   pr_data_clear_xfer_pool();
 
   session.xfer.p = make_sub_pool(session.pool);
-  pr_pool_tag(session.xfer.p, "data transfer pool");
+  pr_pool_tag(session.xfer.p, "Data Transfer pool");
 
   session.xfer.filename = pstrdup(session.xfer.p, filename);
   session.xfer.direction = direction;

diff --git a/src/main.c b/src/main.c
@@ -797,7 +797,8 @@ static cmd_rec *make_ftp_cmd(pool *p, char *buf, size_t buflen, int flags) {
   }
 
   subpool = make_sub_pool(p);
-  cmd = (cmd_rec *) pcalloc(subpool, sizeof(cmd_rec));
+  pr_pool_tag(subpool, "make_ftp_cmd pool");
+  cmd = pcalloc(subpool, sizeof(cmd_rec));
   cmd->pool = subpool;
   cmd->tmp_pool = NULL;
   cmd->stash_index = -1;

diff --git a/tests/t/lib/ProFTPD/Tests/Commands/MLSD.pm b/tests/t/lib/ProFTPD/Tests/Commands/MLSD.pm
@@ -122,6 +122,11 @@ my $TESTS = {
     test_class => [qw(bug forking)],
   },
 
+  mlsd_wide_dir => {
+    order => ++$order,
+    test_class => [qw(bug forking)],
+  },
+
   # XXX Plenty of other tests needed: params, maxfiles, maxdirs, depth, etc
 };
 
@@ -3040,4 +3045,143 @@ sub mlsd_symlinked_dir_showsymlinks_off_bug3859 {
   unlink($log_file);
 }
 
+sub mlsd_wide_dir {
+  my $self = shift;
+  my $tmpdir = $self->{tmpdir};
+  my $setup = test_setup($tmpdir, 'cmds');
+
+  my $test_dir = File::Spec->rel2abs("$tmpdir/test.d");
+  mkpath($test_dir);
+
+  my $expected = {
+    '.' => 1,
+    '..' => 1,
+  };
+
+  my $max_nfiles = 500;
+  for (my $i = 0; $i < $max_nfiles; $i++) {
+    my $test_filename = 'SomeReallyLongAndObnoxiousTestFileNameTemplate' . $i;
+
+    # The expected hash is used later for verifying the results of the READDIR
+    $expected->{$test_filename} = 1;
+
+    my $test_path = File::Spec->rel2abs("$test_dir/$test_filename");
+
+    if (open(my $fh, "> $test_path")) {
+      close($fh);
+
+    } else {
+      die("Can't open $test_path: $!");
+    }
+  }
+
+  my $config = {
+    PidFile => $setup->{pid_file},
+    ScoreboardFile => $setup->{scoreboard_file},
+    SystemLog => $setup->{log_file},
+    TraceLog => $setup->{log_file},
+    Trace => 'pool:20',
+
+    AuthUserFile => $setup->{auth_user_file},
+    AuthGroupFile => $setup->{auth_group_file},
+
+    IfModules => {
+      'mod_delay.c' => {
+        DelayEngine => 'off',
+      },
+    },
+  };
+
+  my ($port, $config_user, $config_group) = config_write($setup->{config_file},
+    $config);
+
+  # Open pipes, for use between the parent and child processes.  Specifically,
+  # the child will indicate when it's done with its test by writing a message
+  # to the parent.
+  my ($rfh, $wfh);
+  unless (pipe($rfh, $wfh)) {
+    die("Can't open pipe: $!");
+  }
+
+  my $ex;
+
+  # Fork child
+  $self->handle_sigchld();
+  defined(my $pid = fork()) or die("Can't fork: $!");
+  if ($pid) {
+    eval {
+      my $client = ProFTPD::TestSuite::FTP->new('127.0.0.1', $port, 1);
+      $client->login($setup->{user}, $setup->{passwd});
+
+      for (my $i = 0; $i < 10; $i++) {
+        my $conn = $client->mlsd_raw($test_dir);
+        unless ($conn) {
+          die("Failed to MLSD: " . $client->response_code() . " " .
+            $client->response_msg());
+        }
+
+        my $buf;
+        my $tmp;
+        while ($conn->read($tmp, 16382, 25)) {
+          $buf .= $tmp;
+          $tmp = '';
+        }
+        eval { $conn->close() };
+
+        if ($ENV{TEST_VERBOSE}) {
+          print STDERR "buf:\n$buf\n";
+        }
+
+        # We have to be careful of the fact that readdir returns directory
+        # entries in an unordered fashion.
+        my $res = {};
+        my $lines = [split(/\r\n/, $buf)];
+        foreach my $line (@$lines) {
+          if ($line =~ /^modify=\S+;perm=\S+;type=\S+;unique=\S+;UNIX\.group=\d+;UNIX\.mode=\d+;UNIX.owner=\d+; (.*?)$/) {
+            $res->{$1} = 1;
+          }
+        }
+
+        my $ok = 1;
+        my $mismatch;
+        foreach my $name (keys(%$res)) {
+          unless (defined($expected->{$name})) {
+            $mismatch = $name;
+            $ok = 0;
+            last;
+          }
+        }
+
+        unless ($ok) {
+          die("Unexpected name '$mismatch' appeared in MLSD data")
+        }
+      }
+
+      $client->quit();
+    };
+
+    if ($@) {
+      $ex = $@;
+    }
+
+    $wfh->print("done\n");
+    $wfh->flush();
+
+  } else {
+    eval { server_wait($setup->{config_file}, $rfh) };
+    if ($@) {
+      warn($@);
+      exit 1;
+    }
+
+    exit 0;
+  }
+
+  # Stop server
+  server_stop($setup->{pid_file});
+  $self->assert_child_ok($pid);
+
+  test_cleanup($setup->{log_file}, $ex);
+}
+
 1;