[Toybox] [PATCH] getfattr.c: fix overlayfs merged dir stuck

[Rob Landley]

On 8/30/22 10:31, Weizhao Ouyang wrote:
> When getfattx reading overlayfs merged dirs, listxattr will got
> different keys_len with zero size and determined size, then it will
> stuck in this while scope. Update the keys_len after the second
> listxattr to fix it.

Do you have a test case I can use to reproduce this? (I've never used xattrs
together with overlayfs before.)

Also, I believe Elliott has a different implementation of getfattr in android
(for some sort of C++ library reasons, I'd have to check my notes), so he'll
probably want the test case there too?

> Signed-off-by: Weizhao Ouyang <o451686892 at gmail.com>
> ---
>  toys/pending/getfattr.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/toys/pending/getfattr.c b/toys/pending/getfattr.c
> index bf2c04c8..aa2c3958 100644
> --- a/toys/pending/getfattr.c
> +++ b/toys/pending/getfattr.c
> @@ -43,15 +43,22 @@ static void do_getfattr(char *file)
>    }
>  
>    // Collect the keys.
> -  while ((keys_len = lister(file, NULL, 0))) {
> -    if (keys_len == -1) perror_msg("listxattr failed");
> -    keys = xmalloc(keys_len);
> -    if (lister(file, keys, keys_len) == keys_len) break;
> +  keys_len = lister(file, NULL, 0);
> +  if (keys_len < 0)
> +    perror_exit("listxattr failed");
> +  else if (keys_len == 0)
> +    return;
> +
> +  keys = xmalloc(keys_len);
> +  keys_len = lister(file, keys, keys_len);
> +  if (keys_len < 0) {
>      free(keys);
> +    perror_exit("listxattr failed");
> +  } else if (keys_len == 0) {
> +    free(keys);
> +    return;
>    }
>  
> -  if (keys_len == 0) return;
> -
>    // Sort the keys.
>    for (key = keys, key_count = 0; key-keys < keys_len; key += strlen(key)+1)
>      key_count++;

Ok, in THEORY the reason for the original loop is that the size could change out
from under us if the filesystem is modified while we're reading it. You're
saying it loops endlessly, because "listxattr will got different keys_len with
zero size and determined size".

Your new unrolled version just _sets_ keys_len to whatever the second one
returns. So on overlayfs, listxattr(file, NULL, 0) and listattr(file, keys, len)
return different values for the same file.

This sounds like a bug in overlayfs you're working around, but if we have to do
that to work with deployed reality fine. The problem is, if the new keys_len is
LONGER than the previous one, then we didn't malloc() enough space for it, and
"sort the keys" below will fall off the end of the buffer.

I'd really like a test case so I can see this in action...

Thanks,

Rob