From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.1 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 Received: from second.openwall.net (second.openwall.net [193.110.157.125]) by inbox.vuxu.org (Postfix) with SMTP id B575C22925 for ; Sat, 27 Jan 2024 20:20:26 +0100 (CET) Received: (qmail 10091 invoked by uid 550); 27 Jan 2024 19:18:07 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 10041 invoked from network); 27 Jan 2024 19:18:07 -0000 Date: Sat, 27 Jan 2024 20:20:10 +0100 From: Szabolcs Nagy To: Rich Felker Cc: Alexander Monakov , "musl@lists.openwall.com" , Andy Caldwell Message-ID: <20240127192010.GB1254592@port70.net> Mail-Followup-To: Rich Felker , Alexander Monakov , "musl@lists.openwall.com" , Andy Caldwell References: <20240125212548.GL4163@brightrain.aerifal.cx> <20240126172716.GN4163@brightrain.aerifal.cx> <20240126195701.GO4163@brightrain.aerifal.cx> <20240127110449.GA1254592@port70.net> <0666848b-fea6-a692-5a7f-6b0daa829f09@ispras.ru> <20240127145608.GP4163@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240127145608.GP4163@brightrain.aerifal.cx> Subject: Re: [musl] RE: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose * Rich Felker [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