* [musl] [PATCH] fix avoidable segfault in catclose @ 2024-01-25 7:09 Ismael Luceno 2024-01-25 14:05 ` Rich Felker 2024-01-25 14:11 ` Markus Wichmann 0 siblings, 2 replies; 17+ messages in thread From: Ismael Luceno @ 2024-01-25 7:09 UTC (permalink / raw) To: musl; +Cc: Rich Felker, 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [musl] [PATCH] fix avoidable segfault in catclose 2024-01-25 7:09 [musl] [PATCH] fix avoidable segfault in catclose Ismael Luceno @ 2024-01-25 14:05 ` Rich Felker 2024-01-25 15:28 ` Ismael Luceno 2024-01-25 14:11 ` Markus Wichmann 1 sibling, 1 reply; 17+ messages in thread From: Rich Felker @ 2024-01-25 14:05 UTC (permalink / raw) To: Ismael Luceno; +Cc: musl 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [musl] [PATCH] fix avoidable segfault in catclose 2024-01-25 14:05 ` Rich Felker @ 2024-01-25 15:28 ` Ismael Luceno 2024-01-25 15:56 ` Rich Felker 0 siblings, 1 reply; 17+ messages in thread From: Ismael Luceno @ 2024-01-25 15:28 UTC (permalink / raw) To: Rich Felker; +Cc: musl <...> > 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/ ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [musl] [PATCH] fix avoidable segfault in catclose 2024-01-25 15:28 ` Ismael Luceno @ 2024-01-25 15:56 ` Rich Felker 0 siblings, 0 replies; 17+ messages in thread From: Rich Felker @ 2024-01-25 15:56 UTC (permalink / raw) To: Ismael Luceno; +Cc: musl 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [musl] [PATCH] fix avoidable segfault in catclose 2024-01-25 7:09 [musl] [PATCH] fix avoidable segfault in catclose Ismael Luceno 2024-01-25 14:05 ` Rich Felker @ 2024-01-25 14:11 ` Markus Wichmann 2024-01-25 15:30 ` Ismael Luceno 2024-01-25 20:10 ` [musl] RE: [EXTERNAL] " Andy Caldwell 1 sibling, 2 replies; 17+ messages in thread From: Markus Wichmann @ 2024-01-25 14:11 UTC (permalink / raw) To: musl; +Cc: Rich Felker, Ismael Luceno 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [musl] [PATCH] fix avoidable segfault in catclose 2024-01-25 14:11 ` Markus Wichmann @ 2024-01-25 15:30 ` Ismael Luceno 2024-01-25 20:10 ` [musl] RE: [EXTERNAL] " Andy Caldwell 1 sibling, 0 replies; 17+ messages in thread From: Ismael Luceno @ 2024-01-25 15:30 UTC (permalink / raw) To: Markus Wichmann; +Cc: musl, Rich Felker 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/ ^ permalink raw reply [flat|nested] 17+ messages in thread
* [musl] RE: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose 2024-01-25 14:11 ` Markus Wichmann 2024-01-25 15:30 ` Ismael Luceno @ 2024-01-25 20:10 ` Andy Caldwell 2024-01-25 21:25 ` Rich Felker 1 sibling, 1 reply; 17+ messages in thread From: Andy Caldwell @ 2024-01-25 20:10 UTC (permalink / raw) To: musl > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [musl] RE: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose 2024-01-25 20:10 ` [musl] RE: [EXTERNAL] " Andy Caldwell @ 2024-01-25 21:25 ` Rich Felker 2024-01-26 17:13 ` Andy Caldwell 0 siblings, 1 reply; 17+ messages in thread From: Rich Felker @ 2024-01-25 21:25 UTC (permalink / raw) To: Andy Caldwell; +Cc: musl 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [musl] RE: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose 2024-01-25 21:25 ` Rich Felker @ 2024-01-26 17:13 ` Andy Caldwell 2024-01-26 17:27 ` Rich Felker 0 siblings, 1 reply; 17+ messages in thread From: Andy Caldwell @ 2024-01-26 17:13 UTC (permalink / raw) To: Rich Felker; +Cc: musl > > > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [musl] RE: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose 2024-01-26 17:13 ` Andy Caldwell @ 2024-01-26 17:27 ` Rich Felker 2024-01-26 19:12 ` Andy Caldwell 0 siblings, 1 reply; 17+ messages in thread From: Rich Felker @ 2024-01-26 17:27 UTC (permalink / raw) To: Andy Caldwell; +Cc: musl 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [musl] RE: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose 2024-01-26 17:27 ` Rich Felker @ 2024-01-26 19:12 ` Andy Caldwell 2024-01-26 19:57 ` Rich Felker 0 siblings, 1 reply; 17+ messages in thread From: Andy Caldwell @ 2024-01-26 19:12 UTC (permalink / raw) To: Rich Felker; +Cc: musl > > > > > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [musl] RE: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose 2024-01-26 19:12 ` Andy Caldwell @ 2024-01-26 19:57 ` Rich Felker 2024-01-26 20:16 ` Andy Caldwell 0 siblings, 1 reply; 17+ messages in thread From: Rich Felker @ 2024-01-26 19:57 UTC (permalink / raw) To: Andy Caldwell; +Cc: musl 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [musl] RE: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose 2024-01-26 19:57 ` Rich Felker @ 2024-01-26 20:16 ` Andy Caldwell 2024-01-27 11:04 ` Szabolcs Nagy 0 siblings, 1 reply; 17+ messages in thread From: Andy Caldwell @ 2024-01-26 20:16 UTC (permalink / raw) To: musl > > > > > > > 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? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [musl] RE: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose 2024-01-26 20:16 ` Andy Caldwell @ 2024-01-27 11:04 ` Szabolcs Nagy 2024-01-27 12:58 ` Alexander Monakov 0 siblings, 1 reply; 17+ messages in thread From: Szabolcs Nagy @ 2024-01-27 11:04 UTC (permalink / raw) To: Andy Caldwell; +Cc: musl * 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [musl] RE: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose 2024-01-27 11:04 ` Szabolcs Nagy @ 2024-01-27 12:58 ` Alexander Monakov 2024-01-27 14:56 ` Rich Felker 0 siblings, 1 reply; 17+ messages in thread From: Alexander Monakov @ 2024-01-27 12:58 UTC (permalink / raw) To: musl; +Cc: Andy Caldwell 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [musl] RE: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose 2024-01-27 12:58 ` Alexander Monakov @ 2024-01-27 14:56 ` Rich Felker 2024-01-27 19:20 ` Szabolcs Nagy 0 siblings, 1 reply; 17+ messages in thread From: Rich Felker @ 2024-01-27 14:56 UTC (permalink / raw) To: Alexander Monakov; +Cc: musl, Andy Caldwell 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [musl] RE: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose 2024-01-27 14:56 ` Rich Felker @ 2024-01-27 19:20 ` Szabolcs Nagy 0 siblings, 0 replies; 17+ messages in thread From: Szabolcs Nagy @ 2024-01-27 19:20 UTC (permalink / raw) To: Rich Felker; +Cc: Alexander Monakov, musl, Andy Caldwell * 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2024-01-27 19:20 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-01-25 7:09 [musl] [PATCH] fix avoidable segfault in catclose Ismael Luceno 2024-01-25 14:05 ` Rich Felker 2024-01-25 15:28 ` Ismael Luceno 2024-01-25 15:56 ` Rich Felker 2024-01-25 14:11 ` Markus Wichmann 2024-01-25 15:30 ` Ismael Luceno 2024-01-25 20:10 ` [musl] RE: [EXTERNAL] " Andy Caldwell 2024-01-25 21:25 ` Rich Felker 2024-01-26 17:13 ` Andy Caldwell 2024-01-26 17:27 ` Rich Felker 2024-01-26 19:12 ` Andy Caldwell 2024-01-26 19:57 ` Rich Felker 2024-01-26 20:16 ` Andy Caldwell 2024-01-27 11:04 ` Szabolcs Nagy 2024-01-27 12:58 ` Alexander Monakov 2024-01-27 14:56 ` Rich Felker 2024-01-27 19:20 ` Szabolcs Nagy
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).