Libguestfs - Mar 2020 - [PATCH libguestfs v2 1/3] daemon: xattr: Refactor code which splits attr names from the kernel.

v1 here:
https://www.redhat.com/archives/libguestfs/2020-March/msg00099.html

This one fixes most of the points picked up in review, and does not
strdup the strings which should keep down memory usage if that is a
concern.

Rich.

The kernel returns xattr names in a slightly peculiar format.  We
parsed this format several times in the code.  Refactor this parsing
so we only do it in one place.
---
 daemon/xattr.c | 123 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 75 insertions(+), 48 deletions(-)

diff --git a/daemon/xattr.c b/daemon/xattr.c
index 5c9f064ce..761f6074b 100644
--- a/daemon/xattr.c
+++ b/daemon/xattr.c
@@ -89,6 +89,32 @@ do_lremovexattr (const char *xattr, const char *path)
   return _removexattr (xattr, path, lremovexattr);
 }
 
+/**
+ * L<listxattr(2)> returns the string C<"foo\0bar\0baz">
of length
+ * C<len>.  (The last string in the list is \0-terminated but the \0
+ * is not included in C<len>).
+ *
+ * This function splits it into a regular list of strings.
+ *
+ * B<Note> that the returned list contains pointers to the original
+ * strings in C<buf> so be careful that you do not double-free them.
+ */
+static char **
+split_attr_names (char *buf, size_t len)
+{
+  size_t i;
+  DECLARE_STRINGSBUF (ret);
+
+  for (i = 0; i < len; i += strlen (&buf[i]) + 1) {
+    if (add_string_nodup (&ret, &buf[i]) == -1)
+      return NULL;
+  }
+  if (end_stringsbuf (&ret) == -1)
+    return NULL;
+
+  return take_stringsbuf (&ret);
+}
+
 static int
 compare_xattrs (const void *vxa1, const void *vxa2)
 {
@@ -106,7 +132,8 @@ getxattrs (const char *path,
 {
   ssize_t len, vlen;
   CLEANUP_FREE char *buf = NULL;
-  size_t i, j;
+  CLEANUP_FREE /* not string list */ char **names = NULL;
+  size_t i;
   guestfs_int_xattr_list *r = NULL;
 
   buf = _listxattrs (path, listxattr, &len);
@@ -114,18 +141,17 @@ getxattrs (const char *path,
     /* _listxattrs issues reply_with_perror already. */
     goto error;
 
+  names = split_attr_names (buf, len);
+  if (names == NULL)
+    goto error;
+
   r = calloc (1, sizeof (*r));
   if (r == NULL) {
     reply_with_perror ("calloc");
     goto error;
   }
 
-  /* What we get from the kernel is a string "foo\0bar\0baz" of
length
-   * len.  First count the strings.
-   */
-  r->guestfs_int_xattr_list_len = 0;
-  for (i = 0; i < (size_t) len; i += strlen (&buf[i]) + 1)
-    r->guestfs_int_xattr_list_len++;
+  r->guestfs_int_xattr_list_len = guestfs_int_count_strings (names);
 
   r->guestfs_int_xattr_list_val      calloc
(r->guestfs_int_xattr_list_len, sizeof (guestfs_int_xattr));
@@ -134,34 +160,34 @@ getxattrs (const char *path,
     goto error;
   }
 
-  for (i = 0, j = 0; i < (size_t) len; i += strlen (&buf[i]) + 1, ++j) {
+  for (i = 0; names[i] != NULL; ++i) {
     CHROOT_IN;
-    vlen = getxattr (path, &buf[i], NULL, 0);
+    vlen = getxattr (path, names[i], NULL, 0);
     CHROOT_OUT;
     if (vlen == -1) {
-      reply_with_perror ("getxattr");
+      reply_with_perror ("getxattr: %s", names[i]);
       goto error;
     }
 
     if (vlen > XATTR_SIZE_MAX) {
       /* The next call to getxattr will fail anyway, so ... */
-      reply_with_error ("extended attribute is too large");
+      reply_with_error ("%s: extended attribute is too large",
names[i]);
       goto error;
     }
 
-    r->guestfs_int_xattr_list_val[j].attrname = strdup (&buf[i]);
-    r->guestfs_int_xattr_list_val[j].attrval.attrval_val = malloc (vlen);
-    r->guestfs_int_xattr_list_val[j].attrval.attrval_len = vlen;
+    r->guestfs_int_xattr_list_val[i].attrname = strdup (names[i]);
+    r->guestfs_int_xattr_list_val[i].attrval.attrval_val = malloc (vlen);
+    r->guestfs_int_xattr_list_val[i].attrval.attrval_len = vlen;
 
-    if (r->guestfs_int_xattr_list_val[j].attrname == NULL ||
-        r->guestfs_int_xattr_list_val[j].attrval.attrval_val == NULL) {
+    if (r->guestfs_int_xattr_list_val[i].attrname == NULL ||
+        r->guestfs_int_xattr_list_val[i].attrval.attrval_val == NULL) {
       reply_with_perror ("malloc");
       goto error;
     }
 
     CHROOT_IN;
-    vlen = getxattr (path, &buf[i],
-                     r->guestfs_int_xattr_list_val[j].attrval.attrval_val,
+    vlen = getxattr (path, names[i],
+                     r->guestfs_int_xattr_list_val[i].attrval.attrval_val,
                      vlen);
     CHROOT_OUT;
     if (vlen == -1) {
@@ -276,7 +302,7 @@ guestfs_int_xattr_list *
 do_internal_lxattrlist (const char *path, char *const *names)
 {
   guestfs_int_xattr_list *ret = NULL;
-  size_t i, j;
+  size_t i;
   size_t k, m, nr_attrs;
   ssize_t len, vlen;
 
@@ -293,6 +319,7 @@ do_internal_lxattrlist (const char *path, char *const
*names)
     void *newptr;
     CLEANUP_FREE char *pathname = NULL;
     CLEANUP_FREE char *buf = NULL;
+    CLEANUP_FREE /* not string list */ char **attrnames = NULL;
 
     /* Be careful in this loop about which errors cause the whole call
      * to abort, and which errors allow us to continue processing
@@ -350,12 +377,10 @@ do_internal_lxattrlist (const char *path, char *const
*names)
     if (len == -1)
       continue; /* not fatal */
 
-    /* What we get from the kernel is a string "foo\0bar\0baz" of
length
-     * len.  First count the strings.
-     */
-    nr_attrs = 0;
-    for (i = 0; i < (size_t) len; i += strlen (&buf[i]) + 1)
-      nr_attrs++;
+    attrnames = split_attr_names (buf, len);
+    if (attrnames == NULL)
+      goto error;
+    nr_attrs = guestfs_int_count_strings (attrnames);
 
     newptr        realloc (ret->guestfs_int_xattr_list_val,
@@ -378,36 +403,36 @@ do_internal_lxattrlist (const char *path, char *const
*names)
       entry[m].attrval.attrval_val = NULL;
     }
 
-    for (i = 0, j = 0; i < (size_t) len; i += strlen (&buf[i]) + 1, ++j)
{
+    for (i = 0; attrnames[i] != NULL; ++i) {
       CHROOT_IN;
-      vlen = lgetxattr (pathname, &buf[i], NULL, 0);
+      vlen = lgetxattr (pathname, attrnames[i], NULL, 0);
       CHROOT_OUT;
       if (vlen == -1) {
-        reply_with_perror ("getxattr");
+        reply_with_perror ("getxattr: %s", attrnames[i]);
         goto error;
       }
 
       if (vlen > XATTR_SIZE_MAX) {
-        reply_with_error ("extended attribute is too large");
+        reply_with_error ("%s: extended attribute is too large",
attrnames[i]);
         goto error;
       }
 
-      entry[j+1].attrname = strdup (&buf[i]);
-      entry[j+1].attrval.attrval_val = malloc (vlen);
-      entry[j+1].attrval.attrval_len = vlen;
+      entry[i+1].attrname = strdup (attrnames[i]);
+      entry[i+1].attrval.attrval_val = malloc (vlen);
+      entry[i+1].attrval.attrval_len = vlen;
 
-      if (entry[j+1].attrname == NULL ||
-          entry[j+1].attrval.attrval_val == NULL) {
+      if (entry[i+1].attrname == NULL ||
+          entry[i+1].attrval.attrval_val == NULL) {
         reply_with_perror ("malloc");
         goto error;
       }
 
       CHROOT_IN;
-      vlen = lgetxattr (pathname, &buf[i],
-                        entry[j+1].attrval.attrval_val, vlen);
+      vlen = lgetxattr (pathname, attrnames[i],
+                        entry[i+1].attrval.attrval_val, vlen);
       CHROOT_OUT;
       if (vlen == -1) {
-        reply_with_perror ("getxattr");
+        reply_with_perror ("getxattr: %s", attrnames[i]);
         goto error;
       }
     }
@@ -510,6 +535,7 @@ copy_xattrs (const char *src, const char *dest)
 {
   ssize_t len, vlen, ret, attrval_len = 0;
   CLEANUP_FREE char *buf = NULL, *attrval = NULL;
+  CLEANUP_FREE /* not string list */ char **names = NULL;
   size_t i;
 
   buf = _listxattrs (src, listxattr, &len);
@@ -517,21 +543,22 @@ copy_xattrs (const char *src, const char *dest)
     /* _listxattrs issues reply_with_perror already. */
     goto error;
 
-  /* What we get from the kernel is a string "foo\0bar\0baz" of
length
-   * len.
-   */
-  for (i = 0; i < (size_t) len; i += strlen (&buf[i]) + 1) {
+  names = split_attr_names (buf, len);
+  if (names == NULL)
+    goto error;
+
+  for (i = 0; names[i] != NULL; ++i) {
     CHROOT_IN;
-    vlen = getxattr (src, &buf[i], NULL, 0);
+    vlen = getxattr (src, names[i], NULL, 0);
     CHROOT_OUT;
     if (vlen == -1) {
-      reply_with_perror ("getxattr: %s, %s", src, &buf[i]);
+      reply_with_perror ("getxattr: %s, %s", src, names[i]);
       goto error;
     }
 
     if (vlen > XATTR_SIZE_MAX) {
       /* The next call to getxattr will fail anyway, so ... */
-      reply_with_error ("extended attribute is too large");
+      reply_with_error ("%s: extended attribute is too large",
names[i]);
       goto error;
     }
 
@@ -546,18 +573,18 @@ copy_xattrs (const char *src, const char *dest)
     }
 
     CHROOT_IN;
-    vlen = getxattr (src, &buf[i], attrval, vlen);
+    vlen = getxattr (src, names[i], attrval, vlen);
     CHROOT_OUT;
     if (vlen == -1) {
-      reply_with_perror ("getxattr: %s, %s", src, &buf[i]);
+      reply_with_perror ("getxattr: %s, %s", src, names[i]);
       goto error;
     }
 
     CHROOT_IN;
-    ret = setxattr (dest, &buf[i], attrval, vlen, 0);
+    ret = setxattr (dest, names[i], attrval, vlen, 0);
     CHROOT_OUT;
     if (ret == -1) {
-      reply_with_perror ("setxattr: %s, %s", dest, &buf[i]);
+      reply_with_perror ("setxattr: %s, %s", dest, names[i]);
       goto error;
     }
   }
-- 
2.25.0

For filtering lists of strings based on a predicate.
---
 daemon/daemon.h |  2 ++
 daemon/utils.c  | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/daemon/daemon.h b/daemon/daemon.h
index 4d7504b3f..9a5ac6d42 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -22,6 +22,7 @@
 #include <stdio.h>
 #include <stdarg.h>
 #include <stdint.h>
+#include <stdbool.h>
 #include <errno.h>
 #include <unistd.h>
 
@@ -74,6 +75,7 @@ extern void free_stringsbuf (struct stringsbuf *sb);
 extern struct stringsbuf split_lines_sb (char *str);
 extern char **split_lines (char *str);
 extern char **empty_list (void);
+extern char **filter_list (bool (*p) (const char *), char **strs);
 extern int is_power_of_2 (unsigned long v);
 extern void trim (char *str);
 extern int parse_btrfsvol (const char *desc, mountable_t *mountable);
diff --git a/daemon/utils.c b/daemon/utils.c
index 1cf1b07f6..21008b40e 100644
--- a/daemon/utils.c
+++ b/daemon/utils.c
@@ -24,6 +24,7 @@
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <stdbool.h>
 #include <string.h>
 #include <unistd.h>
 #include <rpc/types.h>
@@ -482,6 +483,31 @@ empty_list (void)
   return ret.argv;
 }
 
+/**
+ * Filter a list of strings.  Returns a newly allocated list of only
+ * the strings where C<p (str) == true>.
+ *
+ * B<Note> it does not copy the strings, be careful not to double-free
+ * them.
+ */
+char **
+filter_list (bool (*p) (const char *str), char **strs)
+{
+  DECLARE_STRINGSBUF (ret);
+  size_t i;
+
+  for (i = 0; strs[i] != NULL; ++i) {
+    if (p (strs[i])) {
+      if (add_string_nodup (&ret, strs[i]) == -1)
+        return NULL;
+    }
+  }
+  if (end_stringsbuf (&ret) == -1)
+    return NULL;
+
+  return take_stringsbuf (&ret);
+}
+
 /**
  * Skip leading and trailing whitespace, updating the original string
  * in-place.
-- 
2.25.0

See comment in code for justification.

Thanks: Yongkui Guo for finding the bug.
---
 daemon/xattr.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/daemon/xattr.c b/daemon/xattr.c
index 761f6074b..3257f241e 100644
--- a/daemon/xattr.c
+++ b/daemon/xattr.c
@@ -19,6 +19,8 @@
 #include <config.h>
 
 #include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
 #include <limits.h>
 #include <unistd.h>
 
@@ -115,6 +117,29 @@ split_attr_names (char *buf, size_t len)
   return take_stringsbuf (&ret);
 }
 
+/* We hide one extended attribute automatically.  This is used by NTFS
+ * to store the compressed contents of a file when using "CompactOS"
+ * (per-file compression).  I justify this by:
+ *
+ * (1) The attribute is only used internally by NTFS.  The actual file
+ * contents are still available.
+ *
+ * (2) It's probably not valid to copy this attribute when copying the
+ * other attributes of a file.  ntfs-3g-system-compression doesn't
+ * support writing compressed files.
+ *
+ * (3) This file isn't readable by the Linux kernel.  Reading it will
+ * always return -E2BIG (RHBZ#1811539).  So we can't read it even if
+ * we wanted to.
+ *
+ * (4) The Linux kernel itself hides other attributes.
+ */
+static bool
+not_hidden_xattr (const char *attrname)
+{
+  return STRNEQ (attrname, "user.WofCompressedData");
+}
+
 static int
 compare_xattrs (const void *vxa1, const void *vxa2)
 {
@@ -132,6 +157,7 @@ getxattrs (const char *path,
 {
   ssize_t len, vlen;
   CLEANUP_FREE char *buf = NULL;
+  CLEANUP_FREE /* not string list */ char **names_unfiltered = NULL;
   CLEANUP_FREE /* not string list */ char **names = NULL;
   size_t i;
   guestfs_int_xattr_list *r = NULL;
@@ -141,7 +167,10 @@ getxattrs (const char *path,
     /* _listxattrs issues reply_with_perror already. */
     goto error;
 
-  names = split_attr_names (buf, len);
+  names_unfiltered = split_attr_names (buf, len);
+  if (names_unfiltered == NULL)
+    goto error;
+  names = filter_list (not_hidden_xattr, names_unfiltered);
   if (names == NULL)
     goto error;
 
@@ -319,6 +348,7 @@ do_internal_lxattrlist (const char *path, char *const
*names)
     void *newptr;
     CLEANUP_FREE char *pathname = NULL;
     CLEANUP_FREE char *buf = NULL;
+    CLEANUP_FREE /* not string list */ char **attrnames_unfiltered = NULL;
     CLEANUP_FREE /* not string list */ char **attrnames = NULL;
 
     /* Be careful in this loop about which errors cause the whole call
@@ -377,7 +407,10 @@ do_internal_lxattrlist (const char *path, char *const
*names)
     if (len == -1)
       continue; /* not fatal */
 
-    attrnames = split_attr_names (buf, len);
+    attrnames_unfiltered = split_attr_names (buf, len);
+    if (attrnames_unfiltered == NULL)
+      goto error;
+    attrnames = filter_list (not_hidden_xattr, attrnames_unfiltered);
     if (attrnames == NULL)
       goto error;
     nr_attrs = guestfs_int_count_strings (attrnames);
@@ -535,6 +568,7 @@ copy_xattrs (const char *src, const char *dest)
 {
   ssize_t len, vlen, ret, attrval_len = 0;
   CLEANUP_FREE char *buf = NULL, *attrval = NULL;
+  CLEANUP_FREE /* not string list */ char **names_unfiltered = NULL;
   CLEANUP_FREE /* not string list */ char **names = NULL;
   size_t i;
 
@@ -543,7 +577,10 @@ copy_xattrs (const char *src, const char *dest)
     /* _listxattrs issues reply_with_perror already. */
     goto error;
 
-  names = split_attr_names (buf, len);
+  names_unfiltered = split_attr_names (buf, len);
+  if (names_unfiltered == NULL)
+    goto error;
+  names = filter_list (not_hidden_xattr, names_unfiltered);
   if (names == NULL)
     goto error;
 
-- 
2.25.0

On Monday, 16 March 2020 14:59:27 CET Richard W.M. Jones
wrote:
> +static char **
> +split_attr_names (char *buf, size_t len)
> +{
> +  size_t i;
> +  DECLARE_STRINGSBUF (ret);
Theoretically speaking, this stringsbuf is leaked on error;
CLEANUP_FREE_STRINGSBUF can't be used though, as it frees also the
strings and not just the string array.

-- 
Pino Toscano

On Monday, 16 March 2020 14:59:28 CET Richard W.M. Jones
wrote:
> diff --git a/daemon/utils.c b/daemon/utils.c
> index 1cf1b07f6..21008b40e 100644
> --- a/daemon/utils.c
> +++ b/daemon/utils.c
> @@ -24,6 +24,7 @@
>  
>  #include <stdio.h>
>  #include <stdlib.h>
> +#include <stdbool.h>
Since daemon.h now includes stdbool.h, this include is reduntand.
> +/**
> + * Filter a list of strings.  Returns a newly allocated list of only
> + * the strings where C<p (str) == true>.
> + *
> + * B<Note> it does not copy the strings, be careful not to
double-free
> + * them.
> + */
> +char **
> +filter_list (bool (*p) (const char *str), char **strs)
> +{
> +  DECLARE_STRINGSBUF (ret);
Theoretically speaking, this stringsbuf is leaked on error;
CLEANUP_FREE_STRINGSBUF can't be used though, as it frees also the
strings and not just the string array.

(Same note as in patch #1)

-- 
Pino Toscano

On Monday, 16 March 2020 14:59:29 CET Richard W.M. Jones
wrote:
> diff --git a/daemon/xattr.c b/daemon/xattr.c
> index 761f6074b..3257f241e 100644
> --- a/daemon/xattr.c
> +++ b/daemon/xattr.c
> @@ -19,6 +19,8 @@
>  #include <config.h>
>  
>  #include <stdio.h>
> +#include <stdlib.h>
> +#include <stdbool.h>
Since daemon.h now includes stdbool.h, this include is redundant.

-- 
Pino Toscano