[Toybox] ls -l broken

[enh]

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