[Toybox] ls -l broken

[enh]

On Wed, Nov 20, 2019 at 9:52 PM enh <enh at google.com> wrote:
>
> On Wed, Nov 20, 2019 at 9:44 PM Rob Landley <rob at landley.net> wrote:
> >
> > On 11/20/19 4:28 PM, enh via Toybox wrote:
> > > [PATCH] dirtree: fix 0b2cfcb8fdea9673f3c2e0940f1b16d5825e16ea.
> > >
> > > 0b2cfcb8fdea9673f3c2e0940f1b16d5825e16ea broke `ls -l` on files that
> > > don't exist.
> > >
> > > We didn't spot this because the corresponding ls test (which did exist)
> > > had accidentally been mangled in a couple of ways.
> >
> > Sigh. THIS is why I hate testing for specific error messages. (I'm guessing
> > different' libc versions are giving different error messages, so this will break
> > again next year.)
> >
> > > This patch fixes the test (and extends it to differentiate between a
> > > couple of possible ways it can go wrong), and fixes the broken path in
> > > dirtree_add_node.
> >
> > I'm flying again today so got up early and brain's screwed up because of it, but
> > I don't understand why you moved this test:
> >
> > +      sym = AT_SYMLINK_NOFOLLOW*!(flags&DIRTREE_SYMFOLLOW), okay = 1;
> >
> >      // stat dangling symlinks
> >      if (fstatat(fd, name, &st, sym)) {
> > -      if (errno != ENOENT
> > -        || (!sym && fstatat(fd, name, &st, AT_SYMLINK_NOFOLLOW)))
> > -      {
> > -        if (flags&DIRTREE_STATLESS) statless++;
> > -        else goto error;
> > -      }
> > +      okay = (errno == ENOENT && !sym &&
> > +              !fstatat(fd, name, &st, AT_SYMLINK_NOFOLLOW));
> > +    }
> > +    if (!okay) {
> > +      if (flags&DIRTREE_STATLESS) statless++;
> > +      else goto error;
> >      }
> >
> > If "okay" can only be zeroed when we go into the first fstatat()'s if() block,
> > what does moving the test to after the block accomplish? You don't use "okay"
> > again outside that stanza...
>
> the point is that before the ls case where the first fstatat fails
> couldn't fall through to the statless++/goto error block. so that
> needed to move out, and then you need a way to tell how you got there.
> (otherwise you _either_ fail the find tests _or_ you fail the [fixed]
> ls tests.)
>
> > Let's see, old test:
> >
> >   if (errno != ENOENT || (!sym && fstatat()))
> >
> > was effectively replaced with:
> >
> >   if (!(errno == ENOENT && !sym && !fstatat()))
> >
> > But a straight inversion of the first test would be...
> >
> >   if (!(errno == ENOENT && (sym || !fstatat())))
> >
> > So it _looks_ like the fix you applied is basically just changing the existing
> > test to:
> >
> >   if (errno != ENOENT || sym || fstatat())
>
> no, the important part is moving the statless++/goto error part out
> far enough that you can get to it whether or not you do a second
> fstatat.
>
> the currently checked in code just carries on with an uninitialized struct stat.
>
> > The intent of "statless" being set was to return "exists but could not stat".
> > So we set it when the first stat fails, but the error isn't ENOENT and we
> > couldn't symYour change will do that when fstatat() fails with an error other
> > than ENOENT, if readdir returns something we can't stat without following symlinks.
> >
> > Sigh. Why is ls defaulting to showing nanoseconds? How does this help? It makes
> > the output way wider and provides no useful information in most cases. (Debian's
> > ls -l doesn't even default to showing _seconds_...)
> >
> > I'm too sleep deprived to think through this right now. (And I broke the
> > individual command builds on monday. Great.)

(and i'm not sure what you mean by this. `export VERBOSE=1 ; make
test_find && make test_ls` was working fine for me today. though
you'll need to take my fix to the ls test to get any benefit from the
latter half.)

> > Rob