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

[Andy Caldwell <andycaldwell@microsoft.com>]

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

A

-----Original Message-----
From: Markus Wichmann <nullplan@gmx.net> 
Sent: Thursday, January 25, 2024 2:12 PM
To: musl@lists.openwall.com
Cc: Rich Felker <dalias@libc.org>; Ismael Luceno <ismael@iodev.co.uk>
Subject: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose

Am Thu, Jan 25, 2024 at 08:09:49AM +0100 schrieb Ismael Luceno:
> catclose may be called with an invalid argument, particularly -1 may 
> be returned by catopen if there's an error.
>

May it, though? My copy of POSIX does not say so. Whenever a function description does not say that you can call a function with invalid arguments, you cannot do so. And it has been musl policy to crash on invalid args since the beginning.

The problem you describe sounds like your app has control flow being
approximately:

nl_catd cat = catopen(...);
if (cat != (nl_catd)-1) {
    use_cat(cat);
}
catclose(cat);

and that is just wrong control flow and can be remedied by just moving one line:

nl_catd cat = catopen(...);
if (cat != (nl_catd)-1) {
    use_cat(cat);
    catclose(cat);
}

BTW, POSIX does not say catclose() is required (or even allowed) to accept (nl_catd)-1 as argument, its description of the return value of
catopen() also says that it is only suitable for use with catclose() when successful.

Ciao,
Markus