Richard W.M. Jones
2020-Mar-12 14:44 UTC
[Libguestfs] [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.
Richard W.M. Jones
2020-Mar-12 14:44 UTC
[Libguestfs] [PATCH libguestfs 1/3] daemon: xattr: Refactor code which splits attr names from the kernel.
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
Richard W.M. Jones
2020-Mar-12 14:44 UTC
[Libguestfs] [PATCH libguestfs 2/3] daemon: Add filter_list utility function.
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
Richard W.M. Jones
2020-Mar-12 14:44 UTC
[Libguestfs] [PATCH libguestfs 3/3] daemon: xattr: Filter out user.WofCompressedData from xattrs (RHBZ#1811539).
--- 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
Richard W.M. Jones
2020-Mar-12 22:51 UTC
Re: [Libguestfs] [PATCH libguestfs 1/3] daemon: xattr: Refactor code which splits attr names from the kernel.
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
Pino Toscano
2020-Mar-13 13:20 UTC
Re: [Libguestfs] [PATCH libguestfs 2/3] daemon: Add filter_list utility function.
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
Pino Toscano
2020-Mar-13 13:28 UTC
Re: [Libguestfs] [PATCH libguestfs 1/3] daemon: xattr: Refactor code which splits attr names from the kernel.
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
Pino Toscano
2020-Mar-13 14:01 UTC
Re: [Libguestfs] [PATCH libguestfs 0/3] daemon: Fix various commands which break on NTFS-3g compressed files.
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
Richard W.M. Jones
2020-Mar-16 12:20 UTC
Re: [Libguestfs] [PATCH libguestfs 0/3] daemon: Fix various commands which break on NTFS-3g compressed files.
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 _listxattrsThis 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
Seemingly Similar Threads
- [PATCH libguestfs v2 1/3] daemon: xattr: Refactor code which splits attr names from the kernel.
- [PATCH libguestfs v2 3/3] daemon: xattr: Filter out user.WofCompressedData from xattrs (RHBZ#1811539).
- [PATCH libguestfs 0/3] daemon: Fix various commands which break on NTFS-3g compressed files.
- [PATCH libguestfs v2 0/3] daemon: Fix various commands which break on NTFS-3g compressed files.
- [PATCH] New API: copy-attributes.