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