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 E62232469F for ; Sat, 27 Jan 2024 15:56:06 +0100 (CET) Received: (qmail 19792 invoked by uid 550); 27 Jan 2024 14:53:48 -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 19760 invoked from network); 27 Jan 2024 14:53:48 -0000 Date: Sat, 27 Jan 2024 09:56:08 -0500 From: Rich Felker To: Alexander Monakov Cc: "musl@lists.openwall.com" , Andy Caldwell Message-ID: <20240127145608.GP4163@brightrain.aerifal.cx> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0666848b-fea6-a692-5a7f-6b0daa829f09@ispras.ru> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] RE: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose 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