[musl] [PATCH] fix avoidable segfault in catclose

[musl] [PATCH] fix avoidable segfault in catclose

[Ismael Luceno]

catclose may be called with an invalid argument, particularly -1 may be
returned by catopen if there's an error.

Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
---
 src/locale/catclose.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/locale/catclose.c b/src/locale/catclose.c
index 54e24dd2163b..af959a58dfb5 100644
--- a/src/locale/catclose.c
+++ b/src/locale/catclose.c
@@ -8,6 +8,8 @@
 
 int catclose (nl_catd catd)
 {
+	if (catd == (nl_catd)-1)
+		return -1;
 	char *map = (char *)catd;
 	munmap(map, V(map+8)+20);
 	return 0;
-- 
2.43.0

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

[Rich Felker]

On Thu, Jan 25, 2024 at 08:09:49AM +0100, Ismael Luceno wrote:
> catclose may be called with an invalid argument, particularly -1 may be
> returned by catopen if there's an error.
> 
> Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
> ---
>  src/locale/catclose.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/locale/catclose.c b/src/locale/catclose.c
> index 54e24dd2163b..af959a58dfb5 100644
> --- a/src/locale/catclose.c
> +++ b/src/locale/catclose.c
> @@ -8,6 +8,8 @@
>  
>  int catclose (nl_catd catd)
>  {
> +	if (catd == (nl_catd)-1)
> +		return -1;
>  	char *map = (char *)catd;
>  	munmap(map, V(map+8)+20);
>  	return 0;
> -- 
> 2.43.0

Generally in musl, we prefer to trap on UB rather than allowing
forward progress, especially when the natural default action without
special casing it is to trap.

Rich

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

[Markus Wichmann]

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

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

[Ismael Luceno]

<...>
> Generally in musl, we prefer to trap on UB rather than allowing
> forward progress, especially when the natural default action without
> special casing it is to trap.

POSIX says: "The catclose() function may fail if: [EBADF] The catalog
descriptor is not valid. ..."

Implying a known invalid descriptor like -1, or an invalidated
descriptor should be handled.

Glibc manual says:
"...  Errors can occur if the catalog descriptor catalog_desc is not
valid in which case errno is set to EBADF."

The linux manpage also says that once closed, the descriptor gets
invalidated, which isn't what we're doing here.

-- 
Ismael Luceno
http://iodev.co.uk/

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

[Ismael Luceno]

On 25/Jan/2024 15:11, Markus Wichmann wrote:
<...>
> 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.

It does imply it, it mentions that it could take invalid descriptors
and that it should return -1 & errno == EBADF.

Also, this is relevant to code found in the wild, not an application
of my own.

-- 
Ismael Luceno
http://iodev.co.uk/

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

[Rich Felker]

On Thu, Jan 25, 2024 at 04:28:47PM +0100, Ismael Luceno wrote:
> <...>
> > Generally in musl, we prefer to trap on UB rather than allowing
> > forward progress, especially when the natural default action without
> > special casing it is to trap.
> 
> POSIX says: "The catclose() function may fail if: [EBADF] The catalog
> descriptor is not valid. ..."
> 
> Implying a known invalid descriptor like -1, or an invalidated
> descriptor should be handled.
> 
> Glibc manual says:
> "...  Errors can occur if the catalog descriptor catalog_desc is not
> valid in which case errno is set to EBADF."
> 
> The linux manpage also says that once closed, the descriptor gets
> invalidated, which isn't what we're doing here.

This is not a "should be handled". "May fail" is always optional and
is a redundant allowance we explicitly do not use when the behavior is
UB. (It's redundant because anything can happen in the event of UB;
you don't need an allowance for that.)

Here, the API is designed around the possibility that cat_t may be
implemented as some sort of descriptor where it's possible/tractable
to identify valid vs invalid values, and presumably that's where the
idea of EBADF came from. But even EBADF in general is a bad idea, and
not something we'd want to implement except where required. EBADF
basically always indicates a dangerous UAF or DF type bug, where
trapping is the preferred behavior.

The philosophy behind this is explained in the *glibc* wiki, where the
relevant text (10.4.1 NULL pointers) was copied from an answer of mine
on Stack Overflow long ago:

https://sourceware.org/glibc/wiki/Style_and_Conventions#Bugs_in_the_user_program

Rich

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

[Andy Caldwell]

> 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

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

[Rich Felker]

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

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

[Andy Caldwell]

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

It's a hypothetical, but something like the following is valid for the compiler to do:

* inline the catclose (e.g. in LTO for a static link)
* consider the `if` statement and ask "what if `cat` is `-1`
* look forward to the pointer dereference (confirming that `cat` can't change in the interim)
* realise that `0x7` is not a valid pointer on the target platform so UB is inevitable if `cat` is `-1`
* optimize out the comparison since UB frees the compiler of any responsibilities

As an example of exactly this kind of UB-at-a-distance happening see https://lwn.net/Articles/342330/.  As compilers/optimizers get better the scope of fallout for UB is growing over time.

> > 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsourcewa
> re.org%2Fglibc%2Fwiki%2FStyle_and_Conventions%23Bugs_in_the_user_progra
> m&data=05%7C02%7Candycaldwell%40microsoft.com%7Cb8a5e6447ca64d9484
> 2d08dc1dec2e01%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63841
> 8147445459351%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQI
> joiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=qxIHR
> sJ3nCRiYxTSdECipLeIxi9khnd7R5HOyr8RCvI%3D&reserved=0
> ?
> 
> 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.

I did read that post (in fact that's what prompted my comment) - and I agree with its sentiment.  Unfortunately for that post's goals UB is non-local in the face of compiler optimizations making trapping reliably when functions are misused near impossible (though I acknowledge the point about `ubsan` which I'd not thought of).  If `libc` functions invoke UB then all bets are off, and it's near impossible for them to validate their arguments (e.g. "a non-freed pointer" is not detectable in any feasible way) in order to explicitly `abort` or similar.  Maybe the intended policy is to order the code of each function to "propagate" UB though as early as possible and hope that causes a fault that can be debugged (or that the user is using `ubsan`).

A

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

[Rich Felker]

On Fri, Jan 26, 2024 at 05:13:13PM +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).
> 
> It's a hypothetical, but something like the following is valid for the compiler to do:
> 
> * inline the catclose (e.g. in LTO for a static link)
> * consider the `if` statement and ask "what if `cat` is `-1`
> * look forward to the pointer dereference (confirming that `cat` can't change in the interim)
> * realise that `0x7` is not a valid pointer on the target platform so UB is inevitable if `cat` is `-1`
> * optimize out the comparison since UB frees the compiler of any responsibilities

You have the logic backwards. In the case where cat==(cat_t)-1,
catclose is not called on the abstract machine, so no conclusions can
be drawn from anything catclose would do.

Rich

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

[Andy Caldwell]

> > > > > 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).
> >
> > It's a hypothetical, but something like the following is valid for the compiler to
> do:
> >
> > * inline the catclose (e.g. in LTO for a static link)
> > * consider the `if` statement and ask "what if `cat` is `-1`
> > * look forward to the pointer dereference (confirming that `cat` can't
> > change in the interim)
> > * realise that `0x7` is not a valid pointer on the target platform so
> > UB is inevitable if `cat` is `-1`
> > * optimize out the comparison since UB frees the compiler of any
> > responsibilities
> 
> You have the logic backwards. In the case where cat==(cat_t)-1, catclose is not
> called on the abstract machine, so no conclusions can be drawn from anything
> catclose would do.

The original code I was working from was:

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

(i.e. an incorrect use of the APIs, but not UB in a "C99 spec" sense).  In that code the `catclose` call is provably inevitable, allowing the compiler to infer properties of `cat` from it.

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

[Rich Felker]

On Fri, Jan 26, 2024 at 07:12:59PM +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).
> > >
> > > It's a hypothetical, but something like the following is valid for the compiler to
> > do:
> > >
> > > * inline the catclose (e.g. in LTO for a static link)
> > > * consider the `if` statement and ask "what if `cat` is `-1`
> > > * look forward to the pointer dereference (confirming that `cat` can't
> > > change in the interim)
> > > * realise that `0x7` is not a valid pointer on the target platform so
> > > UB is inevitable if `cat` is `-1`
> > > * optimize out the comparison since UB frees the compiler of any
> > > responsibilities
> > 
> > You have the logic backwards. In the case where cat==(cat_t)-1, catclose is not
> > called on the abstract machine, so no conclusions can be drawn from anything
> > catclose would do.
> 
> The original code I was working from was:
> 
> ```
> nl_catd cat = catopen(...);
> if (cat != (nl_catd)-1) {
>     use_cat(cat);
> }
> catclose(cat);
> ```
> 
> (i.e. an incorrect use of the APIs, but not UB in a "C99 spec"
> sense). In that code the `catclose` call is provably inevitable,
> allowing the compiler to infer properties of `cat` from it.

Ah, okay, at least now that makes sense. But indeed it is undefined:

  "Each of the following statements shall apply to all functions
   unless explicitly stated otherwise in the detailed descriptions
   that follow:

   1. If an argument to a function has an invalid value (such as a
      value outside the domain of the function, or a pointer outside
      the address space of the program, or a null pointer), the
      behavior is undefined.

   ..."

https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_01

So I guess what you're saying is that, in the case where an erroneous
program like the above has undefined behavior, the compiler could make
a transformation such that the effect of the UB is seen at a point
different from where it logically occurs. (This is the norm for UB.)
In particular, despite cat being -1 from a failed catopen, you might
see use_cat being called with a seemingly impossible argument.

Exacerbating the degree to which UB can become non-localized is one of
the expected effects of LTO, and arguably a good reason not to use LTO
for debugging. I don't see a lot of value in trying to prevent this in
isolated cases when it's going to happen all over the place anyway for
other reasons.

Rich

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

[Andy Caldwell]

> > > > > > > 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).
> > > >
> > > > It's a hypothetical, but something like the following is valid for
> > > > the compiler to
> > > do:
> > > >
> > > > * inline the catclose (e.g. in LTO for a static link)
> > > > * consider the `if` statement and ask "what if `cat` is `-1`
> > > > * look forward to the pointer dereference (confirming that `cat`
> > > > can't change in the interim)
> > > > * realise that `0x7` is not a valid pointer on the target platform
> > > > so UB is inevitable if `cat` is `-1`
> > > > * optimize out the comparison since UB frees the compiler of any
> > > > responsibilities
> > >
> > > You have the logic backwards. In the case where cat==(cat_t)-1,
> > > catclose is not called on the abstract machine, so no conclusions
> > > can be drawn from anything catclose would do.
> >
> > The original code I was working from was:
> >
> > ```
> > nl_catd cat = catopen(...);
> > if (cat != (nl_catd)-1) {
> >     use_cat(cat);
> > }
> > catclose(cat);
> > ```
> >
> > (i.e. an incorrect use of the APIs, but not UB in a "C99 spec"
> > sense). In that code the `catclose` call is provably inevitable,
> > allowing the compiler to infer properties of `cat` from it.
>
> Ah, okay, at least now that makes sense. But indeed it is undefined:
>
>   "Each of the following statements shall apply to all functions
>    unless explicitly stated otherwise in the detailed descriptions
>    that follow:
>
>    1. If an argument to a function has an invalid value (such as a
>       value outside the domain of the function, or a pointer outside
>       the address space of the program, or a null pointer), the
>       behavior is undefined.
>
>    ..."
>
> https://pubs.ope/
> ngroup.org%2Fonlinepubs%2F9699919799%2Ffunctions%2FV2_chap02.html%23t
> ag_15_01&data=05%7C02%7Candycaldwell%40microsoft.com%7C7cfdfca2ce32
> 4149a5cd08dc1ea8f669%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7
> C638418958276620853%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD
> AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata
> =0lEpsKknyTB3xULpxaOBZS6UIzI1rCG4D86U9z6P5Xc%3D&reserved=0
>
> So I guess what you're saying is that, in the case where an erroneous program like
> the above has undefined behavior, the compiler could make a transformation such
> that the effect of the UB is seen at a point different from where it logically occurs.
> (This is the norm for UB.) In particular, despite cat being -1 from a failed catopen,
> you might see use_cat being called with a seemingly impossible argument.

Yes, this - the details aren't particularly interesting but the key is that "invoke UB" is not the same as "crash/trap".  I'm also contrasting this to the comments in the glibc wiki and Markus's synopsis (from the earlier email) that "it has been musl policy to crash on invalid args since the beginning" - in the face of UB, musl (and presumably also glibc) _doesn't_ crash/trap, nor does it "fail early and catastrophically" it instead "propagates the UB".  In debug builds these are often equivalent, but the specific path to UB might not be seen in a debug build, and only be seen in production where the non-locality of UB effects are at their worst.

> Exacerbating the degree to which UB can become non-localized is one of the
> expected effects of LTO, and arguably a good reason not to use LTO for debugging.
> I don't see a lot of value in trying to prevent this in isolated cases when it's going
> to happen all over the place anyway for other reasons.

I think that's reasonable, but it disagrees with the "policy" as described in the glibc wiki/your SO post/etc. (maybe I'm being pedantic, but I read "it shall fail early and catastrophically" as "it will definitely fail, and in a useful way, even in optimized builds" which isn't always the case and often can't be).  Maybe I'm simply suggesting that that position needs some clarification/caveating?

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

[Szabolcs Nagy]

* Andy Caldwell <andycaldwell@microsoft.com> [2024-01-26 20:16:58 +0000]:
> > > > > > > > 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).
> > > > >
> > > > > It's a hypothetical, but something like the following is valid for
> > > > > the compiler to
> > > > do:
> > > > >
> > > > > * inline the catclose (e.g. in LTO for a static link)
> > > > > * consider the `if` statement and ask "what if `cat` is `-1`
> > > > > * look forward to the pointer dereference (confirming that `cat`
> > > > > can't change in the interim)
> > > > > * realise that `0x7` is not a valid pointer on the target platform
> > > > > so UB is inevitable if `cat` is `-1`
> > > > > * optimize out the comparison since UB frees the compiler of any
> > > > > responsibilities
> > > >
> > > > You have the logic backwards. In the case where cat==(cat_t)-1,
> > > > catclose is not called on the abstract machine, so no conclusions
> > > > can be drawn from anything catclose would do.
> > >
> > > The original code I was working from was:
> > >
> > > ```
> > > nl_catd cat = catopen(...);
> > > if (cat != (nl_catd)-1) {
> > >     use_cat(cat);
> > > }
> > > catclose(cat);
> > > ```
> > >
> > > (i.e. an incorrect use of the APIs, but not UB in a "C99 spec"
> > > sense). In that code the `catclose` call is provably inevitable,
> > > allowing the compiler to infer properties of `cat` from it.
> >
> > Ah, okay, at least now that makes sense. But indeed it is undefined:
> >
> >   "Each of the following statements shall apply to all functions
> >    unless explicitly stated otherwise in the detailed descriptions
> >    that follow:
> >
> >    1. If an argument to a function has an invalid value (such as a
> >       value outside the domain of the function, or a pointer outside
> >       the address space of the program, or a null pointer), the
> >       behavior is undefined.
> >
> >    ..."
> >
> > https://pubs.ope/
> > ngroup.org%2Fonlinepubs%2F9699919799%2Ffunctions%2FV2_chap02.html%23t
> > ag_15_01&data=05%7C02%7Candycaldwell%40microsoft.com%7C7cfdfca2ce32
> > 4149a5cd08dc1ea8f669%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7
> > C638418958276620853%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD
> > AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata
> > =0lEpsKknyTB3xULpxaOBZS6UIzI1rCG4D86U9z6P5Xc%3D&reserved=0
> >
> > So I guess what you're saying is that, in the case where an erroneous program like
> > the above has undefined behavior, the compiler could make a transformation such
> > that the effect of the UB is seen at a point different from where it logically occurs.
> > (This is the norm for UB.) In particular, despite cat being -1 from a failed catopen,
> > you might see use_cat being called with a seemingly impossible argument.
> 
> Yes, this - the details aren't particularly interesting but the key is that "invoke UB"
> is not the same as "crash/trap".  I'm also contrasting this to the comments in the
> glibc wiki and Markus's synopsis (from the earlier email) that "it has been musl policy
> to crash on invalid args since the beginning" - in the face of UB, musl (and presumably
> also glibc) _doesn't_ crash/trap, nor does it "fail early and catastrophically" it
> instead "propagates the UB".  In debug builds these are often equivalent, but the
> specific path to UB might not be seen in a debug build, and only be seen in production
> where the non-locality of UB effects are at their worst.

i think you are still looking at this the wrong way:

- the original code has ub.
- so anything can happen.
- whatever libc does, still anything can happen.
- adding a check p==-1 in libc does not change anything.
(the ub already happens in the caller. a compiler can even remove the
call since it can know about catclose semantics.)

given these facts on the theoretical level, we can look pragmatically at
the actual transformations a compiler would likely do and we find that
an invalid NULL+n dereference in practice is almost surely an immediate
crash (on linux with dynamic linking or static linking without lto this
is not only likely but actually guaranteed by existing toolchains) which
is the best possible outcome for debugging, meanwhile an extra check in
libc is worse: the code continues and misbehaves somewhere else.

given that glibc cannot be built with lto, and static linked lto musl
libc.a is not widely deployed since it would only be usable with the
exact same compiler version that built it, i think your objections are
quite theoretical.

> 
> > Exacerbating the degree to which UB can become non-localized is one of the
> > expected effects of LTO, and arguably a good reason not to use LTO for debugging.
> > I don't see a lot of value in trying to prevent this in isolated cases when it's going
> > to happen all over the place anyway for other reasons.
> 
> I think that's reasonable, but it disagrees with the "policy" as described in
> the glibc wiki/your SO post/etc. (maybe I'm being pedantic, but I read "it
> shall fail early and catastrophically" as "it will definitely fail, and in a
> useful way, even in optimized builds" which isn't always the case and often
> can't be).  Maybe I'm simply suggesting that that position needs some
> clarification/caveating?

if the c runtime could give guarantees about ub, we would have memory
safe c implementations. i don't think users expect such guarantees but
i guess one clould add extra wording there to point it out.

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

[Alexander Monakov]

On Sat, 27 Jan 2024, Szabolcs Nagy wrote:

> > Yes, this - the details aren't particularly interesting but the key is that "invoke UB"
> > is not the same as "crash/trap".  I'm also contrasting this to the comments in the
> > glibc wiki and Markus's synopsis (from the earlier email) that "it has been musl policy
> > to crash on invalid args since the beginning" - in the face of UB, musl (and presumably
> > also glibc) _doesn't_ crash/trap, nor does it "fail early and catastrophically" it
> > instead "propagates the UB".  In debug builds these are often equivalent, but the
> > specific path to UB might not be seen in a debug build, and only be seen in production
> > where the non-locality of UB effects are at their worst.
> 
> i think you are still looking at this the wrong way:
> 
> - the original code has ub.
> - so anything can happen.
> - whatever libc does, still anything can happen.
> - adding a check p==-1 in libc does not change anything.
> (the ub already happens in the caller. a compiler can even remove the
> call since it can know about catclose semantics.)
> 
> given these facts on the theoretical level, we can look pragmatically at
> the actual transformations a compiler would likely do and we find that
> an invalid NULL+n dereference in practice is almost surely an immediate
> crash (on linux with dynamic linking or static linking without lto this
> is not only likely but actually guaranteed by existing toolchains) which
> is the best possible outcome for debugging, meanwhile an extra check in
> libc is worse: the code continues and misbehaves somewhere else.

I don't think this follows. I believe the suggestion was to have

    if (catd == (nl_catd)-1) a_crash();

which is the opposite of "continuing and misbehaving".

Alexander

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

[Rich Felker]

On Sat, Jan 27, 2024 at 03:58:02PM +0300, Alexander Monakov wrote:
> 
> On Sat, 27 Jan 2024, Szabolcs Nagy wrote:
> 
> > > Yes, this - the details aren't particularly interesting but the key is that "invoke UB"
> > > is not the same as "crash/trap".  I'm also contrasting this to the comments in the
> > > glibc wiki and Markus's synopsis (from the earlier email) that "it has been musl policy
> > > to crash on invalid args since the beginning" - in the face of UB, musl (and presumably
> > > also glibc) _doesn't_ crash/trap, nor does it "fail early and catastrophically" it
> > > instead "propagates the UB".  In debug builds these are often equivalent, but the
> > > specific path to UB might not be seen in a debug build, and only be seen in production
> > > where the non-locality of UB effects are at their worst.
> > 
> > i think you are still looking at this the wrong way:
> > 
> > - the original code has ub.
> > - so anything can happen.
> > - whatever libc does, still anything can happen.
> > - adding a check p==-1 in libc does not change anything.
> > (the ub already happens in the caller. a compiler can even remove the
> > call since it can know about catclose semantics.)
> > 
> > given these facts on the theoretical level, we can look pragmatically at
> > the actual transformations a compiler would likely do and we find that
> > an invalid NULL+n dereference in practice is almost surely an immediate
> > crash (on linux with dynamic linking or static linking without lto this
> > is not only likely but actually guaranteed by existing toolchains) which
> > is the best possible outcome for debugging, meanwhile an extra check in
> > libc is worse: the code continues and misbehaves somewhere else.
> 
> I don't think this follows. I believe the suggestion was to have
> 
>     if (catd == (nl_catd)-1) a_crash();
> 
> which is the opposite of "continuing and misbehaving".

Indeed, that was my understanding too. The reason I don't like this is
that it's a lot of spurious code (not in a single place, but if we did
stuff like this consistently everywhere) and on top of that it
actively makes debugging more difficult. You have to trace the flow of
execution from the trapping instruction back to the branch that led to
it and figure out why that was taken rather than seeing the invalid
pointer directly in the instruction operand register (and knowing even
before you see it, e.g. just with strace not even gdb, that the cause
was an invalid pointer).

Rich

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

[Szabolcs Nagy]

* Rich Felker <dalias@libc.org> [2024-01-27 09:56:08 -0500]:
> On Sat, Jan 27, 2024 at 03:58:02PM +0300, Alexander Monakov wrote:
> > 
> > On Sat, 27 Jan 2024, Szabolcs Nagy wrote:
> > 
> > > > Yes, this - the details aren't particularly interesting but the key is that "invoke UB"
> > > > is not the same as "crash/trap".  I'm also contrasting this to the comments in the
> > > > glibc wiki and Markus's synopsis (from the earlier email) that "it has been musl policy
> > > > to crash on invalid args since the beginning" - in the face of UB, musl (and presumably
> > > > also glibc) _doesn't_ crash/trap, nor does it "fail early and catastrophically" it
> > > > instead "propagates the UB".  In debug builds these are often equivalent, but the
> > > > specific path to UB might not be seen in a debug build, and only be seen in production
> > > > where the non-locality of UB effects are at their worst.
> > > 
> > > i think you are still looking at this the wrong way:
> > > 
> > > - the original code has ub.
> > > - so anything can happen.
> > > - whatever libc does, still anything can happen.
> > > - adding a check p==-1 in libc does not change anything.
> > > (the ub already happens in the caller. a compiler can even remove the
> > > call since it can know about catclose semantics.)
> > > 
> > > given these facts on the theoretical level, we can look pragmatically at
> > > the actual transformations a compiler would likely do and we find that
> > > an invalid NULL+n dereference in practice is almost surely an immediate
> > > crash (on linux with dynamic linking or static linking without lto this
> > > is not only likely but actually guaranteed by existing toolchains) which
> > > is the best possible outcome for debugging, meanwhile an extra check in
> > > libc is worse: the code continues and misbehaves somewhere else.
> > 
> > I don't think this follows. I believe the suggestion was to have
> > 
> >     if (catd == (nl_catd)-1) a_crash();
> > 
> > which is the opposite of "continuing and misbehaving".
> 
> Indeed, that was my understanding too. The reason I don't like this is
> that it's a lot of spurious code (not in a single place, but if we did

sorry i thought we were debating 'return EBADF'.

> stuff like this consistently everywhere) and on top of that it
> actively makes debugging more difficult. You have to trace the flow of
> execution from the trapping instruction back to the branch that led to
> it and figure out why that was taken rather than seeing the invalid

in a sense this is still 'continue and misbehave somewhere else':
if many code paths end in a_crash and the compiler merges them,
then we can lose the branch location, so we would need an a_crash
that the compiler does not merge to be able to find the branch.

> pointer directly in the instruction operand register (and knowing even
> before you see it, e.g. just with strace not even gdb, that the cause
> was an invalid pointer).
> 
> Rich