[Toybox] ls -l broken

[Rob Landley]

On 11/20/19 11:52 PM, enh wrote:
> On Wed, Nov 20, 2019 at 9:44 PM Rob Landley <rob at landley.net> wrote:
>> 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.

You only set statless++ when the first fstatat() fails. There's nothing for it
to "fall through" for when the first fstatat() didn't fail.

This is expressed in your new logic too: okay = 1; if (false) {not entered;};
if (!okay) {can never be reached.}

What did moving it down accomplish?

> 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

The find tests are all passing with (or without) the simpler fix?

> _or_ you fail the [fixed] ls tests.)

I'm confused by "fixed" here: you switch the test from looking for the word
"missing" to the word "non-existent" and on debian TEST_HOST the error message is:

  $ ls does-not-exist
  ls: cannot access 'does-not-exist': No such file or directory

"A strange game. The only winning move is not to play." - Wargames

I did this:

-testing "missing" "$IN && ls does-not-exist 2>err ; grep -q 'ls:.*missing.*: No
-such file' err || echo missing error; $OUT" "" "" ""
+testing "missing" "$IN && ls does-not-exist 2>&1 >/dev/null | grep -o
does-not-exist; $OUT" "does-not-exist\n" "" ""

But the problem is I'm not reproducing your issue _without_ changes to
lib/dirtree.c:

  $ ./ls missing
  ls: missing: No such file or directory
  $ echo $/
  1

It wasn't failing for me before? How do I reproduce the failure?

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

But it never triggers if you didn't go into the first if() block.

> the currently checked in code just carries on with an uninitialized struct stat.

Hmmm, uninitialized value being used. That would be a potential bug I couldn't
necessarily reproduce, yes. Let's see...

Ah! If the first stat fails but sym was set, we don't do the second attempt and
that _also_ skips the else goto error. Yeah, that's a bug.

Try now?

Rob