From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail8.parnet.fi (mail8.parnet.fi [77.234.108.134]) by sourceware.org (Postfix) with ESMTPS id 805CE385C32C for ; Wed, 24 Aug 2022 10:46:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 805CE385C32C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=martin.st Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=martin.st Received: from mail9.parnet.fi (mail9.parnet.fi [77.234.108.21]) by mail8.parnet.fi with ESMTP id 27OAk5F7001224-27OAk5F8001224; Wed, 24 Aug 2022 13:46:05 +0300 Received: from foo.martin.st (host-97-187.parnet.fi [77.234.97.187]) by mail9.parnet.fi (Postfix) with ESMTPS id 1DE07A144B; Wed, 24 Aug 2022 13:46:05 +0300 (EEST) Date: Wed, 24 Aug 2022 13:46:04 +0300 (EEST) From: =?ISO-8859-15?Q?Martin_Storsj=F6?= To: Jan Beulich cc: binutils@sourceware.org Subject: Re: [PATCH v5] ld: Make archive member file extension comparisons case insensitive when cross compiling too In-Reply-To: Message-ID: <9b767c3b-2690-863c-e7dd-17fa477ee482@martin.st> References: <20220824100434.7797-1-martin@martin.st> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="8323329-1911721348-1661337965=:1659" X-FE-Policy-ID: 3:14:2:SYSTEM X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-1911721348-1661337965=:1659 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8BIT On Wed, 24 Aug 2022, Jan Beulich wrote: > On 24.08.2022 12:04, Martin Storsjö wrote: >> @@ -1857,7 +1880,7 @@ gld${EMULATION_NAME}_unrecognized_file (lang_input_statement_type *entry ATTRIBU >> #ifdef DLL_SUPPORT >> const char *ext = entry->filename + strlen (entry->filename) - 4; >> >> - if (filename_cmp (ext, ".def") == 0 || filename_cmp (ext, ".DEF") == 0) >> + if (fileext_cmp (ext, "def") == 0) >> { >> pe_def_file = def_file_parse (entry->filename, pe_def_file); >> > > The pre-existing code doesn't look safe here (and I did overlook the > lack of strrchr() here when writing my earlier reply). There's a > buffer underflow for file names shorter than 4 characters. Oh, indeed! > And I'm inclined to say that ".def" on its own isn't a .def-file, but > a file without any extension. (This applies to all other cases you > change as well.) > > If I was touching all of this anyway, I'd be inclined to address both > issues as a "side effect" of the patch. But of course it's not a > requirement; it can easily be a separate, later patch. Or you could > also elect to switch to using strrchr() here (thus allowing code to > be dropped from the new function with callers all adding 1 to the > pointer they pass), but leave the "not really an extension" part > alone. I think simplicity is key here; whatever keeps the code the simplest is best, since exactly how we handle hypothetical cases here probably shouldn't matter much in practice, as long as it's safe. At the third call site, I also noticed that we're lacking a null pointer check before invoking the comparison, compared to the other ones - I'll amend that too. (It's possible that it's in a place where we know for sure that it's non-null, but it's not immediately obvious when looking at it with the context of the patch at least.) // Martin --8323329-1911721348-1661337965=:1659-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail8.parnet.fi (mail8.parnet.fi [77.234.108.134]) by sourceware.org (Postfix) with ESMTPS id 805CE385C32C for ; Wed, 24 Aug 2022 10:46:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 805CE385C32C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=martin.st Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=martin.st Received: from mail9.parnet.fi (mail9.parnet.fi [77.234.108.21]) by mail8.parnet.fi with ESMTP id 27OAk5F7001224-27OAk5F8001224; Wed, 24 Aug 2022 13:46:05 +0300 Received: from foo.martin.st (host-97-187.parnet.fi [77.234.97.187]) by mail9.parnet.fi (Postfix) with ESMTPS id 1DE07A144B; Wed, 24 Aug 2022 13:46:05 +0300 (EEST) Date: Wed, 24 Aug 2022 13:46:04 +0300 (EEST) From: =?ISO-8859-15?Q?Martin_Storsj=F6?= To: Jan Beulich cc: binutils@sourceware.org Subject: Re: [PATCH v5] ld: Make archive member file extension comparisons case insensitive when cross compiling too In-Reply-To: Message-ID: <9b767c3b-2690-863c-e7dd-17fa477ee482@martin.st> References: <20220824100434.7797-1-martin@martin.st> MIME-Version: 1.0 X-FE-Policy-ID: 3:14:2:SYSTEM X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: binutils@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Aug 2022 10:46:08 -0000 Message-ID: <20220824104604.U0WtRWh8bQ_-mZDN105wfl5eINH7iwM5g4lvhBHJxUk@z> On Wed, 24 Aug 2022, Jan Beulich wrote: > On 24.08.2022 12:04, Martin Storsjö wrote: >> @@ -1857,7 +1880,7 @@ gld${EMULATION_NAME}_unrecognized_file (lang_input_statement_type *entry ATTRIBU >> #ifdef DLL_SUPPORT >> const char *ext = entry->filename + strlen (entry->filename) - 4; >> >> - if (filename_cmp (ext, ".def") == 0 || filename_cmp (ext, ".DEF") == 0) >> + if (fileext_cmp (ext, "def") == 0) >> { >> pe_def_file = def_file_parse (entry->filename, pe_def_file); >> > > The pre-existing code doesn't look safe here (and I did overlook the > lack of strrchr() here when writing my earlier reply). There's a > buffer underflow for file names shorter than 4 characters. Oh, indeed! > And I'm inclined to say that ".def" on its own isn't a .def-file, but > a file without any extension. (This applies to all other cases you > change as well.) > > If I was touching all of this anyway, I'd be inclined to address both > issues as a "side effect" of the patch. But of course it's not a > requirement; it can easily be a separate, later patch. Or you could > also elect to switch to using strrchr() here (thus allowing code to > be dropped from the new function with callers all adding 1 to the > pointer they pass), but leave the "not really an extension" part > alone. I think simplicity is key here; whatever keeps the code the simplest is best, since exactly how we handle hypothetical cases here probably shouldn't matter much in practice, as long as it's safe. At the third call site, I also noticed that we're lacking a null pointer check before invoking the comparison, compared to the other ones - I'll amend that too. (It's possible that it's in a place where we know for sure that it's non-null, but it's not immediately obvious when looking at it with the context of the patch at least.) // Martin