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 3AE65230AB for ; Thu, 25 Jan 2024 22:25:47 +0100 (CET) Received: (qmail 22256 invoked by uid 550); 25 Jan 2024 21:23:34 -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 22224 invoked from network); 25 Jan 2024 21:23:33 -0000 Date: Thu, 25 Jan 2024 16:25:49 -0500 From: Rich Felker To: Andy Caldwell Cc: "musl@lists.openwall.com" Message-ID: <20240125212548.GL4163@brightrain.aerifal.cx> References: <20240125070950.28673-1-ismael@iodev.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] RE: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose 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