Re: [musl] RE: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose

[Rich Felker <dalias@libc.org>]

On Thu, Jan 25, 2024 at 08:10:30PM +0000, Andy Caldwell wrote:
> > And it has been musl policy to crash on invalid args since the beginning.
> 
> The current implementation doesn't (necessarily) crash/trap on an
> invalid argument, instead it invokes (C-language spec-defined) UB
> itself (it dereferences `(uint32_t*)((char*)cat) + 8)`, which, in
> the case of the `-1` handle is the address 0x7, which in turn, not
> being a valid address, is UB to dereference). If you're lucky (or
> are compiling without optimizations/inlining) the compiler will emit
> a MOV that will trigger an access violation and hence a SEGV, if

In general, it's impossible to test for "is this pointer valid?"

There are certain special cases we could test for, but unless there is
a particularly convincing reason that they could lead to runaway wrong
execution/vulnerabilities prior to naturally trapping, we have not
considered littering the code with these kinds of checks to be a
worthwhile trade-off.

> you're unlucky the compiler will make wild assumptions about the
> value of the variable passed as the arg (and for example in your
> first code snippet, simply delete the `if` statement, meaning
> `use_cat` gets called even when `catopen` fails potentially
> corrupting user data/state).

I have no idea what you're talking about there. The compiler cannot
make that kind of transformation (lifting code that could produce
undefined behavior, side effects, etc. out of a conditional).

> Crashing loudly (which requires _not_
> invoking UB) on known bad inputs (a test against `-1` isn't exactly
> expensive) feels like it meets the "musl policy" better than the
> current code.

Letting the caller-directed UB "propagate through" to corresponding UB
inside the implementation gives maximum debugging visibility of the
root cause of the crash, and lets whoever's building link up their
preferred form of instrumentation (e.g. -fsanitize=undefined).

Did you read the linked text
https://sourceware.org/glibc/wiki/Style_and_Conventions#Bugs_in_the_user_program
?

Yes that is the glibc wiki, but I'm the original author of the text
that was based on, which was in turn based on existing practice in
musl. As written it's about NULL, but the same applies to (cat_t)-1,
MAP_FAILED, and invalid pointers in general.

Rich