[PATCH libguestfs 0/3] daemon: Fix various commands which break on NTFS-3g compressed files.
https://bugzilla.redhat.com/show_bug.cgi?id=1811539 Commands including virt-diff which read extended attributes will sometimes fail on NTFS filesystems that are using system compressed. The reason is complex, see comment 5 of the bug linked above. This patch filters out the troublesome xattr. For justification, see the comment I added in patch 3. Patch 1 & 2 are refactoring. I was on the verge of rewriting the whole file in OCaml, but OCaml doesn't have access to the relevant system calls so it would have been a bunch more work and not necessarily any simpler. Thanks: Yongkui Guo for identifying the bug. Rich.
The kernel returns xattr names in a slightly peculiar format. We parsed this format several times in the code. Refactoring this parsing so we only do it in one place. --- daemon/xattr.c | 126 ++++++++++++++++++++++++++++++------------------- 1 file changed, 78 insertions(+), 48 deletions(-) diff --git a/daemon/xattr.c b/daemon/xattr.c index 5c9f064ce..482df9af0 100644 --- a/daemon/xattr.c +++ b/daemon/xattr.c @@ -89,6 +89,35 @@ 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. + * + * Note the caller must free the returned string list. + */ +static char ** +split_attr_names (const char *buf, size_t len) +{ + size_t i; + DECLARE_STRINGSBUF (ret); + + for (i = 0; i < len; i += strlen (&buf[i]) + 1) { + if (add_string (&ret, &buf[i]) == -1) { + free_stringsbuf (&ret); + return NULL; + } + } + if (end_stringsbuf (&ret) == -1) { + free_stringsbuf (&ret); + return NULL; + } + + return take_stringsbuf (&ret); +} + static int compare_xattrs (const void *vxa1, const void *vxa2) { @@ -106,7 +135,8 @@ getxattrs (const char *path, { ssize_t len, vlen; CLEANUP_FREE char *buf = NULL; - size_t i, j; + CLEANUP_FREE_STRING_LIST char **names = NULL; + size_t i; guestfs_int_xattr_list *r = NULL; buf = _listxattrs (path, listxattr, &len); @@ -114,18 +144,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 +163,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 ("%s: getxattr", 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 (&buf[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 +305,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 +322,7 @@ do_internal_lxattrlist (const char *path, char *const *names) void *newptr; CLEANUP_FREE char *pathname = NULL; CLEANUP_FREE char *buf = NULL; + CLEANUP_FREE_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 +380,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 +406,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 ("%s: getxattr", 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 ("%s: getxattr", attrnames[i]); goto error; } } @@ -510,6 +538,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_STRING_LIST char **names = NULL; size_t i; buf = _listxattrs (src, listxattr, &len); @@ -517,21 +546,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 +576,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
On Thu, Mar 12, 2020 at 02:44:46PM +0000, Richard W.M. Jones wrote:
- 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 (&buf[i]);
I guess our testing doesn't cover this function, because it would certainly have crashed here. &buf[i] should be names[i]. I have updated my local copy. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Thursday, 12 March 2020 15:44:46 CET Richard W.M. Jones wrote:
+static char ** +split_attr_names (const char *buf, size_t len) +{ + size_t i; + DECLARE_STRINGSBUF (ret);
Declare this as CLEANUP_FREE_STRINGSBUF, so there is no need for the manual calls to free_stringsbuf on failure. take_stringsbuf at the return is fine, as it will hand over the resources to the caller.
CHROOT_OUT; if (vlen == -1) { - reply_with_perror ("getxattr"); + reply_with_perror ("%s: getxattr", names[i]);
IIRC "what: details" is more common, so "getxattr: %s". (Same for the other ocurrences.)
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 (&buf[i]);
names[i] [you already fixed this, I just amended this message before sending it, as I started to write it yesterday]
@@ -378,36 +406,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) {
Considering we have nr_attrs at this point, we can use it. -- Pino Toscano -- Pino Toscano
For filtering lists of strings based on a predicate. --- daemon/daemon.h | 1 + daemon/utils.c | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/daemon/daemon.h b/daemon/daemon.h index 4d7504b3f..a16953f23 100644 --- a/daemon/daemon.h +++ b/daemon/daemon.h @@ -74,6 +74,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 (int (*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..1fc8d2c80 100644 --- a/daemon/utils.c +++ b/daemon/utils.c @@ -482,6 +482,28 @@ empty_list (void) return ret.argv; } +/** + * Filter a list of strings. Returns a newly allocated list of only + * the strings where C<p (str) != 0>. + */ +char ** +filter_list (int (*p) (const char *str), char **strs) +{ + DECLARE_STRINGSBUF (ret); + size_t i; + + for (i = 0; strs[i] != NULL; ++i) { + if (p (strs[i]) != 0) { + if (add_string (&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
On Thursday, 12 March 2020 15:44:47 CET Richard W.M. Jones wrote:
For filtering lists of strings based on a predicate. --- daemon/daemon.h | 1 + daemon/utils.c | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+)
diff --git a/daemon/daemon.h b/daemon/daemon.h index 4d7504b3f..a16953f23 100644 --- a/daemon/daemon.h +++ b/daemon/daemon.h @@ -74,6 +74,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 (int (*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..1fc8d2c80 100644 --- a/daemon/utils.c +++ b/daemon/utils.c @@ -482,6 +482,28 @@ empty_list (void) return ret.argv; }
+/** + * Filter a list of strings. Returns a newly allocated list of only + * the strings where C<p (str) != 0>.
Maybe use bool as return value instead?
+ */ +char ** +filter_list (int (*p) (const char *str), char **strs) +{ + DECLARE_STRINGSBUF (ret);
Declare this as CLEANUP_FREE_STRINGSBUF, so the stringsbuf is properly cleaned on failure. take_stringsbuf at the return is fine, as it will hand over the resources to the caller. -- Pino Toscano
--- daemon/xattr.c | 41 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/daemon/xattr.c b/daemon/xattr.c index 482df9af0..82fb0ebc8 100644 --- a/daemon/xattr.c +++ b/daemon/xattr.c @@ -118,6 +118,29 @@ split_attr_names (const 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 int +not_hidden_xattr (const char *attrname) +{ + return STRNEQ (attrname, "user.WofCompressedData"); +} + static int compare_xattrs (const void *vxa1, const void *vxa2) { @@ -135,6 +158,7 @@ getxattrs (const char *path, { ssize_t len, vlen; CLEANUP_FREE char *buf = NULL; + CLEANUP_FREE_STRING_LIST char **names_unfiltered = NULL; CLEANUP_FREE_STRING_LIST char **names = NULL; size_t i; guestfs_int_xattr_list *r = NULL; @@ -144,7 +168,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; @@ -322,6 +349,7 @@ do_internal_lxattrlist (const char *path, char *const *names) void *newptr; CLEANUP_FREE char *pathname = NULL; CLEANUP_FREE char *buf = NULL; + CLEANUP_FREE_STRING_LIST char **attrnames_unfiltered = NULL; CLEANUP_FREE_STRING_LIST char **attrnames = NULL; /* Be careful in this loop about which errors cause the whole call @@ -380,7 +408,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); @@ -538,6 +569,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_STRING_LIST char **names_unfiltered = NULL; CLEANUP_FREE_STRING_LIST char **names = NULL; size_t i; @@ -546,7 +578,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 Thursday, 12 March 2020 15:44:45 CET Richard W.M. Jones wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1811539
Commands including virt-diff which read extended attributes will sometimes fail on NTFS filesystems that are using system compressed. The reason is complex, see comment 5 of the bug linked above.
This patch filters out the troublesome xattr. For justification, see the comment I added in patch 3.
The idea is fine, however I'm slightly concerned about the increased memory usage due to the split + filtering done. Now that I took a look at the code again (wow, last time I touched it was more than 5 years ago), an alternative approach could be: - refactor do_internal_lxattrlist to use _listxattrs - edit/replace in-place the buffer to-be-returned by _listxattrs filtering out the names we do not want This way the rest of the existing code needs almost no changes. -- Pino Toscano
On Fri, Mar 13, 2020 at 03:01:00PM +0100, Pino Toscano wrote:
On Thursday, 12 March 2020 15:44:45 CET Richard W.M. Jones wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1811539
Commands including virt-diff which read extended attributes will sometimes fail on NTFS filesystems that are using system compressed. The reason is complex, see comment 5 of the bug linked above.
This patch filters out the troublesome xattr. For justification, see the comment I added in patch 3.
The idea is fine, however I'm slightly concerned about the increased memory usage due to the split + filtering done.
Now that I took a look at the code again (wow, last time I touched it was more than 5 years ago), an alternative approach could be: - refactor do_internal_lxattrlist to use _listxattrs
This isn't totally straightforward because the functions handle errors in slightly different ways (of course we could add a very specific bool parameter for that case, but ...)
- edit/replace in-place the buffer to-be-returned by _listxattrs filtering out the names we do not want This way the rest of the existing code needs almost no changes.
So my alternate optimization would be to do the list filtering as per the patch I posted, but not strdup the strings themselves. It achieves mainly the same end as what you're saying but is a lot safer and clearer than trying to edit in place the weird "multi-string" that the kernel returns. I'll try that. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
participants (2)
-
Pino Toscano -
Richard W.M. Jones