[Toybox] llist_traverse() and dirtree_path()

[Rob Landley]

On 07/17/2012 12:14 AM, Ashwini Sharma wrote:
> Hi Rob,
> 
>  You have modified llist_free to llist_traverse().
> 
> In llist_traverse()
> 
> while (list) {
>         void *pop = llist_pop(&list);
>         using(pop);
> 
>         // End doubly linked list too.
>         if (list==pop) break;
>     }
> 
> 'using' is used without checking it. This is potential crash, if NULL
> is passed to llist_tarverse().
> 
> I do understand that, why should someone make a call to traverse, if
> not wanting to use the elements. But for a safe programming, I think
> checking the variable would be better.

I have a TOYBOX_DEBUG config symbol that lets me put in debugging code I
don't want compiled in during normal use, but I generally shy away from
asserts.

An assert doesn't make the code _right_. It adds code that should never
trigger, but will bit-rot as assumptions around it change until the
assert triggers when it shouldn't. I've hit multiple cases where the bug
_was_ the assert, and the fix was to remove it. Asserts make the program
bigger and more complicated without contributing to its function.

There's no such thing as "safe programming" in C. We are using power
tools here with all the sharp bits exposed. Nature of the beast.

That's why I think it's better to add tests to the test suite to
exercise all the code paths and try to hit the various limits than to
add code that should never trigger. Either the program is wrong in a way
that either the test suite or regular usage will catch, or it isn't.
Yes, there may wind up being an exploitable code path that we didn't
think of, but that's true whether your talking about asserts, test suite
tests, or simple code inspection. Making the code have as few lines as
possible so it does exactly what it needs and no more is the most
reliable way to reduce that sort of bug: that way we can see exactly
what it's doing.

In this case passing NULL is going to immediately segfault the program,
which is about as obvious as an assert. While it's possible to (somewhat
laboriously) set up memory mappings in susv4 compatability mode that put
a code page over the null page so dereferencing null can call an
arbitrary function, this pointer isn't user data that's fed in so a code
path with a null pointer here should always, obviously break. It
shouldn't be a lurking hard to trigger exploitable corner case, it
should break obviously and the command in question _never_work_.

I could add an error_exit() that what function the problem is happening
in, ala:

        if (CFG_TOYBOX_DEBUG && !using)
                error_exit("llist_traverse called with NULL using");

But that doesn't really make it faster to track down, because there's no
backtrace. Admittedly glibc does have a backtrace() function, but it's
simultaneously non-portable and darn near useless (doesn't print
anthing, you have to write a print function around it. Which they give
in the man page, but not in the library), and I'd need portability.h
glue to make it available even under TOYBOX_DEBUG, which is why I
haven't bothered.

> For dirtree_path(), I had requested you earlier also, can we modify
> the '/' appending logic to append the '/' only in case its not there
> at the end else its fine.
> Like,
> if (len) {
>         if(path[len -1] != '/') path[len++]='/';
>     }

Ah, right. Good catch.

  http://landley.net/hg/toybox/rev/629

(And it's still getting the total: wrong in ls, which I just rant to
test it.  Sigh...)

Rob
-- 
GNU/Linux isn't: Linux=GPLv2, GNU=GPLv3+, they can't share code.
Either it's "mere aggregation", or a license violation.  Pick one.

 1342589350.0