From a13f9b06370a6fef6f671f5aa7c23df0f0fd542e Mon Sep 17 00:00:00 2001 From: "Richard W.M. Jones" Date: Thu, 12 Mar 2020 13:57:06 +0000 Subject: [PATCH] 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. Refactor this parsing so we only do it in one place. (cherry picked from commit 5c175fe73264bbf1d3ef79bb066dfb6aff902ad1) --- daemon/xattr.c | 127 ++++++++++++++++++++++++++++++------------------- 1 file changed, 79 insertions(+), 48 deletions(-) diff --git a/daemon/xattr.c b/daemon/xattr.c index bc5c2df97..3e1144963 100644 --- a/daemon/xattr.c +++ b/daemon/xattr.c @@ -89,6 +89,36 @@ do_lremovexattr (const char *xattr, const char *path) return _removexattr (xattr, path, lremovexattr); } +/** + * L returns the string C<"foo\0bar\0baz"> of length + * C. (The last string in the list is \0-terminated but the \0 + * is not included in C). + * + * This function splits it into a regular list of strings. + * + * B that the returned list contains pointers to the original + * strings in C 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) { + free (ret.argv); + return NULL; + } + } + if (end_stringsbuf (&ret) == -1) { + free (ret.argv); + return NULL; + } + + return take_stringsbuf (&ret); +} + static int compare_xattrs (const void *vxa1, const void *vxa2) { @@ -106,7 +136,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 +145,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 +164,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 +306,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 +323,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 +381,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 +407,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 +539,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 +547,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 +577,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.18.4