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

[enh]

On Wed, Aug 31, 2022 at 3:27 AM Rob Landley <rob at landley.net> wrote:

> 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?
>

no, i sent you this implementation :-)

i think you're thinking of modprobe, which is parallelized and used as a
library inside init directly?


> > 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.
>

+David Anderson <dvander at google.com> --- are you aware of this behavior?
(sounds like a bug to me too, but you know more about fs stuff than i do...)


> I'd really like a test case so I can see this in action...
>
> Thanks,
>
> Rob
> _______________________________________________
> Toybox mailing list
> Toybox at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20220831/38bbc215/attachment.htm>