From b97b90779d5ea261d5e737f073bb4ec5dc546511 Mon Sep 17 00:00:00 2001

From: Laszlo Ersek <lersek@redhat.com>

Date: Mon, 2 May 2022 10:56:00 +0200

Subject: [PATCH] guestfs_readdir(): rewrite with FileOut transfer, to lift

 protocol limit

Currently the guestfs_readdir() API can not list long directories, due to

it sending back the whole directory listing in a single guestfs protocol

response, which is limited to GUESTFS_MESSAGE_MAX (approx. 4MB) in size.

Introduce the "internal_readdir" action, for transferring the directory

listing from the daemon to the library through a FileOut parameter.

Rewrite guestfs_readdir() on top of this new internal function:

- The new "internal_readdir" action is a daemon action. Do not repurpose

  the "readdir" proc_nr (138) for "internal_readdir", as some distros ship

  the binary appliance to their users, and reusing the proc_nr could

  create a mismatch between library & appliance with obscure symptoms.

  Replace the old proc_nr (138) with a new proc_nr (511) instead; a

  mismatch would then produce a clear error message. Assume the new action

  will first be released in libguestfs-1.48.2.

- Turn "readdir" from a daemon action into a non-daemon one. Call the

  daemon action guestfs_internal_readdir() manually, receive the FileOut

  parameter into a temp file, then deserialize the dirents array from the

  temp file.

This patch sneakily fixes an independent bug, too. In the pre-patch

do_readdir() function [daemon/readdir.c], when readdir() returns NULL, we

don't distinguish "end of directory stream" from "readdir() failed". This

rewrite fixes this problem -- I didn't see much value separating out the

fix for the original do_readdir().

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1674392

Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Message-Id: <20220502085601.15012-2-lersek@redhat.com>

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

(cherry picked from commit 45b7f1736b64e9f0741e21e5a9d83a837bd863bf)

---

 TODO                      |   8 ---

 daemon/readdir.c          | 132 +++++++++++++++++++-------------------

 generator/actions_core.ml | 127 +++++++++++++++++++-----------------

 generator/proc_nr.ml      |   2 +-

 lib/MAX_PROC_NR           |   2 +-

 lib/Makefile.am           |   1 +

 lib/readdir.c             | 131 +++++++++++++++++++++++++++++++++++++

 7 files changed, 267 insertions(+), 136 deletions(-)

 create mode 100644 lib/readdir.c

diff --git a/TODO b/TODO

index a50f7d73c..513e55f92 100644

--- a/TODO

+++ b/TODO

@@ -484,14 +484,6 @@ this approach works, it doesn't solve the MBR problem, so likely we'd

 have to write a library for that (or perhaps go back to sfdisk but

 using a very abstracted interface over sfdisk).

-Reimplement some APIs to avoid protocol limits

-----------------------------------------------

-

-Mostly this item was done (eg. commits a69f44f56f and before).  The

-most notable API with a protocol limit remaining is:

-

- - guestfs_readdir

-

 hivex

 -----

diff --git a/daemon/readdir.c b/daemon/readdir.c

index e488f93e7..9ab0b0aec 100644

--- a/daemon/readdir.c

+++ b/daemon/readdir.c

@@ -16,77 +16,67 @@

  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.

  */

-#include <config.h>

+#include <config.h> /* HAVE_STRUCT_DIRENT_D_TYPE */

-#include <stdio.h>

-#include <stdlib.h>

-#include <string.h>

-#include <unistd.h>

-#include <dirent.h>

+#include <dirent.h>    /* readdir() */

+#include <errno.h>     /* errno */

+#include <rpc/xdr.h>   /* xdrmem_create() */

+#include <stdio.h>     /* perror() */

+#include <stdlib.h>    /* malloc() */

+#include <sys/types.h> /* opendir() */

-#include "daemon.h"

-#include "actions.h"

+#include "daemon.h" /* reply_with_perror() */

-static void

-free_int_dirent_list (guestfs_int_dirent *p, size_t len)

+/* Has one FileOut parameter. */

+int

+do_internal_readdir (const char *dir)

 {

-  size_t i;

+  int ret;

+  DIR *dirstream;

+  void *xdr_buf;

+  XDR xdr;

-  for (i = 0; i < len; ++i) {

-    free (p[i].name);

-  }

-  free (p);

-}

-

-guestfs_int_dirent_list *

-do_readdir (const char *path)

-{

-  guestfs_int_dirent_list *ret;

-  guestfs_int_dirent v;

-  DIR *dir;

-  struct dirent *d;

-  size_t i;

-

-  ret = malloc (sizeof *ret);

-  if (ret == NULL) {

-    reply_with_perror ("malloc");

-    return NULL;

-  }

-

-  ret->guestfs_int_dirent_list_len = 0;

-  ret->guestfs_int_dirent_list_val = NULL;

+  /* Prepare to fail. */

+  ret = -1;

   CHROOT_IN;

-  dir = opendir (path);

+  dirstream = opendir (dir);

   CHROOT_OUT;

-  if (dir == NULL) {

-    reply_with_perror ("opendir: %s", path);

-    free (ret);

-    return NULL;

+  if (dirstream == NULL) {

+    reply_with_perror ("opendir: %s", dir);

+    return ret;

   }

-  i = 0;

-  while ((d = readdir (dir)) != NULL) {

-    guestfs_int_dirent *p;

+  xdr_buf = malloc (GUESTFS_MAX_CHUNK_SIZE);

+  if (xdr_buf == NULL) {

+    reply_with_perror ("malloc");

+    goto close_dir;

+  }

+  xdrmem_create (&xdr, xdr_buf, GUESTFS_MAX_CHUNK_SIZE, XDR_ENCODE);

+

+  /* Send an "OK" reply, before starting the file transfer. */

+  reply (NULL, NULL);

+

+  /* From this point on, we can only report errors by canceling the file

+   * transfer.

+   */

+  for (;;) {

+    struct dirent *d;

+    guestfs_int_dirent v;

+

+    errno = 0;

+    d = readdir (dirstream);

+    if (d == NULL) {

+      if (errno == 0)

+        ret = 0;

+      else

+        perror ("readdir");

-    p = realloc (ret->guestfs_int_dirent_list_val,

-                 sizeof (guestfs_int_dirent) * (i+1));

-    v.name = strdup (d->d_name);

-    if (!p || !v.name) {

-      reply_with_perror ("allocate");

-      if (p) {

-        free_int_dirent_list (p, i);

-      } else {

-        free_int_dirent_list (ret->guestfs_int_dirent_list_val, i);

-      }

-      free (v.name);

-      free (ret);

-      closedir (dir);

-      return NULL;

+      break;

     }

-    ret->guestfs_int_dirent_list_val = p;

+    v.name = d->d_name;

     v.ino = d->d_ino;

 #ifdef HAVE_STRUCT_DIRENT_D_TYPE

     switch (d->d_type) {

@@ -104,19 +94,29 @@ do_readdir (const char *path)

     v.ftyp = 'u';

 #endif

-    ret->guestfs_int_dirent_list_val[i] = v;

+    if (!xdr_guestfs_int_dirent (&xdr, &v)) {

+      fprintf (stderr, "xdr_guestfs_int_dirent failed\n");

+      break;

+    }

-    i++;

+    if (send_file_write (xdr_buf, xdr_getpos (&xdr)) != 0)

+      break;

+

+    xdr_setpos (&xdr, 0);

   }

-  ret->guestfs_int_dirent_list_len = i;

+  /* Finish or cancel the transfer. Note that if (ret == -1) because the library

+   * canceled, we still need to cancel back!

+   */

+  send_file_end (ret == -1);

-  if (closedir (dir) == -1) {

-    reply_with_perror ("closedir");

-    free (ret->guestfs_int_dirent_list_val);

-    free (ret);

-    return NULL;

-  }

+  xdr_destroy (&xdr;;

+  free (xdr_buf);

+

+close_dir:

+  if (closedir (dirstream) == -1)

+    /* Best we can do here is log an error. */

+    perror ("closedir");

   return ret;

 }

diff --git a/generator/actions_core.ml b/generator/actions_core.ml

index dc12fdc33..807150615 100644

--- a/generator/actions_core.ml

+++ b/generator/actions_core.ml

@@ -141,6 +141,66 @@ only useful for printing debug and internal error messages.

 For more information on states, see L<guestfs(3)>." };

+  { defaults with

+    name = "readdir"; added = (1, 0, 55);

+    style = RStructList ("entries", "dirent"), [String (Pathname, "dir")], [];

+    progress = true; cancellable = true;

+    shortdesc = "read directories entries";

+    longdesc = "\

+This returns the list of directory entries in directory C<dir>.

+

+All entries in the directory are returned, including C<.> and

+C<..>.  The entries are I<not> sorted, but returned in the same

+order as the underlying filesystem.

+

+Also this call returns basic file type information about each

+file.  The C<ftyp> field will contain one of the following characters:

+

+=over 4

+

+=item 'b'

+

+Block special

+

+=item 'c'

+

+Char special

+

+=item 'd'

+

+Directory

+

+=item 'f'

+

+FIFO (named pipe)

+

+=item 'l'

+

+Symbolic link

+

+=item 'r'

+

+Regular file

+

+=item 's'

+

+Socket

+

+=item 'u'

+

+Unknown file type

+

+=item '?'

+

+The L<readdir(3)> call returned a C<d_type> field with an

+unexpected value

+

+=back

+

+This function is primarily intended for use by programs.  To

+get a simple list of names, use C<guestfs_ls>.  To get a printable

+directory for human consumption, use C<guestfs_ll>." };

+

   { defaults with

     name = "version"; added = (1, 0, 58);

     style = RStruct ("version", "version"), [], [];

@@ -3939,66 +3999,6 @@ L<umask(2)>, C<guestfs_mknod>, C<guestfs_mkdir>.

 This call returns the previous umask." };

-  { defaults with

-    name = "readdir"; added = (1, 0, 55);

-    style = RStructList ("entries", "dirent"), [String (Pathname, "dir")], [];

-    protocol_limit_warning = true;

-    shortdesc = "read directories entries";

-    longdesc = "\

-This returns the list of directory entries in directory C<dir>.

-

-All entries in the directory are returned, including C<.> and

-C<..>.  The entries are I<not> sorted, but returned in the same

-order as the underlying filesystem.

-

-Also this call returns basic file type information about each

-file.  The C<ftyp> field will contain one of the following characters:

-

-=over 4

-

-=item 'b'

-

-Block special

-

-=item 'c'

-

-Char special

-

-=item 'd'

-

-Directory

-

-=item 'f'

-

-FIFO (named pipe)

-

-=item 'l'

-

-Symbolic link

-

-=item 'r'

-

-Regular file

-

-=item 's'

-

-Socket

-

-=item 'u'

-

-Unknown file type

-

-=item '?'

-

-The L<readdir(3)> call returned a C<d_type> field with an

-unexpected value

-

-=back

-

-This function is primarily intended for use by programs.  To

-get a simple list of names, use C<guestfs_ls>.  To get a printable

-directory for human consumption, use C<guestfs_ll>." };

-

   { defaults with

     name = "getxattrs"; added = (1, 0, 59);

     style = RStructList ("xattrs", "xattr"), [String (Pathname, "path")], [];

@@ -9713,4 +9713,11 @@ C<guestfs_cryptsetup_open>.  The C<device> parameter must be

 the name of the mapping device (ie. F</dev/mapper/mapname>)

 and I<not> the name of the underlying block device." };

+  { defaults with

+    name = "internal_readdir"; added = (1, 48, 2);

+    style = RErr, [String (Pathname, "dir"); String (FileOut, "filename")], [];

+    visibility = VInternal;

+    shortdesc = "read directories entries";

+    longdesc = "Internal function for readdir." };

+

 ]

diff --git a/generator/proc_nr.ml b/generator/proc_nr.ml

index b20672ff0..bdced51c9 100644

--- a/generator/proc_nr.ml

+++ b/generator/proc_nr.ml

@@ -152,7 +152,6 @@ let proc_nr = [

 135, "mknod_b";

 136, "mknod_c";

 137, "umask";

-138, "readdir";

 139, "sfdiskM";

 140, "zfile";

 141, "getxattrs";

@@ -514,6 +513,7 @@ let proc_nr = [

 508, "cryptsetup_open";

 509, "cryptsetup_close";

 510, "internal_list_rpm_applications";

+511, "internal_readdir";

 ]

 (* End of list.  If adding a new entry, add it at the end of the list

diff --git a/lib/MAX_PROC_NR b/lib/MAX_PROC_NR

index 2bc4cd64b..c0556fb20 100644

--- a/lib/MAX_PROC_NR

+++ b/lib/MAX_PROC_NR

@@ -1 +1 @@

-510

+511

diff --git a/lib/Makefile.am b/lib/Makefile.am

index 144c45588..212bcb94a 100644

--- a/lib/Makefile.am

+++ b/lib/Makefile.am

@@ -105,6 +105,7 @@ libguestfs_la_SOURCES = \

 	private-data.c \

 	proto.c \

 	qemu.c \

+	readdir.c \

 	rescue.c \

 	stringsbuf.c \

 	structs-compare.c \

diff --git a/lib/readdir.c b/lib/readdir.c

new file mode 100644

index 000000000..9cb0d7cf6

--- /dev/null

+++ b/lib/readdir.c

@@ -0,0 +1,131 @@

+/* libguestfs

+ * Copyright (C) 2016-2022 Red Hat Inc.

+ *

+ * This library is free software; you can redistribute it and/or

+ * modify it under the terms of the GNU Lesser General Public

+ * License as published by the Free Software Foundation; either

+ * version 2 of the License, or (at your option) any later version.

+ *

+ * This library is distributed in the hope that it will be useful,

+ * but WITHOUT ANY WARRANTY; without even the implied warranty of

+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

+ * Lesser General Public License for more details.

+ *

+ * You should have received a copy of the GNU Lesser General Public

+ * License along with this library; if not, write to the Free Software

+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

+ */

+

+#include <config.h> /* UNIX_PATH_MAX, needed by "guestfs-internal.h" */

+

+#include <rpc/xdr.h> /* xdrstdio_create() */

+#include <stdint.h>  /* UINT32_MAX */

+#include <stdio.h>   /* fopen() */

+#include <string.h>  /* memset() */

+

+#include "guestfs.h"                  /* guestfs_internal_readdir() */

+#include "guestfs_protocol.h"         /* guestfs_int_dirent */

+#include "guestfs-internal.h"         /* guestfs_int_make_temp_path() */

+#include "guestfs-internal-actions.h" /* guestfs_impl_readdir */

+

+struct guestfs_dirent_list *

+guestfs_impl_readdir (guestfs_h *g, const char *dir)

+{

+  struct guestfs_dirent_list *ret;

+  char *tmpfn;

+  FILE *f;

+  off_t fsize;

+  XDR xdr;

+  struct guestfs_dirent_list *dirents;

+  uint32_t alloc_entries;

+  size_t alloc_bytes;

+

+  /* Prepare to fail. */

+  ret = NULL;

+

+  tmpfn = guestfs_int_make_temp_path (g, "readdir", NULL);

+  if (tmpfn == NULL)

+    return ret;

+

+  if (guestfs_internal_readdir (g, dir, tmpfn) == -1)

+    goto drop_tmpfile;

+

+  f = fopen (tmpfn, "r");

+  if (f == NULL) {

+    perrorf (g, "fopen: %s", tmpfn);

+    goto drop_tmpfile;

+  }

+

+  if (fseeko (f, 0, SEEK_END) == -1) {

+    perrorf (g, "fseeko");

+    goto close_tmpfile;

+  }

+  fsize = ftello (f);

+  if (fsize == -1) {

+    perrorf (g, "ftello");

+    goto close_tmpfile;

+  }

+  if (fseeko (f, 0, SEEK_SET) == -1) {

+    perrorf (g, "fseeko");

+    goto close_tmpfile;

+  }

+

+  xdrstdio_create (&xdr, f, XDR_DECODE);

+

+  dirents = safe_malloc (g, sizeof *dirents);

+  dirents->len = 0;

+  alloc_entries = 8;

+  alloc_bytes = alloc_entries * sizeof *dirents->val;

+  dirents->val = safe_malloc (g, alloc_bytes);

+

+  while (xdr_getpos (&xdr) < fsize) {

+    guestfs_int_dirent v;

+    struct guestfs_dirent *d;

+

+    if (dirents->len == alloc_entries) {

+      if (alloc_entries > UINT32_MAX / 2 || alloc_bytes > (size_t)-1 / 2) {

+        error (g, "integer overflow");

+        goto free_dirents;

+      }

+      alloc_entries *= 2u;

+      alloc_bytes *= 2u;

+      dirents->val = safe_realloc (g, dirents->val, alloc_bytes);

+    }

+

+    /* Decoding does not work unless the target buffer is zero-initialized. */

+    memset (&v, 0, sizeof v);

+    if (!xdr_guestfs_int_dirent (&xdr, &v)) {

+      error (g, "xdr_guestfs_int_dirent failed");

+      goto free_dirents;

+    }

+

+    d = &dirents->val[dirents->len];

+    d->ino = v.ino;

+    d->ftyp = v.ftyp;

+    d->name = v.name; /* transfer malloc'd string to "d" */

+

+    dirents->len++;

+  }

+

+  /* Success; transfer "dirents" to "ret". */

+  ret = dirents;

+  dirents = NULL;

+

+  /* Clean up. */

+  xdr_destroy (&xdr;;

+

+free_dirents:

+  guestfs_free_dirent_list (dirents);

+

+close_tmpfile:

+  fclose (f);

+

+drop_tmpfile:

+  /* In case guestfs_internal_readdir() failed, it may or may not have created

+   * the temporary file.

+   */

+  unlink (tmpfn);

+  free (tmpfn);

+

+  return ret;

+}

-- 

2.31.1