From: "Martin Storsjö" <martin@martin.st>
To: Jan Beulich <jbeulich@suse.com>
Cc: binutils@sourceware.org
Subject: Re: [PATCH v5] ld: Make archive member file extension comparisons case insensitive when cross compiling too
Date: Wed, 24 Aug 2022 13:46:04 +0300 (EEST) [thread overview]
Message-ID: <9b767c3b-2690-863c-e7dd-17fa477ee482@martin.st> (raw)
In-Reply-To: <de4513b6-a44a-8f8d-aef3-4504853849a0@suse.com>
[-- Attachment #1: Type: text/plain, Size: 1842 bytes --]
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
WARNING: multiple messages have this Message-ID
From: "Martin Storsjö" <martin@martin.st>
To: Jan Beulich <jbeulich@suse.com>
Cc: binutils@sourceware.org
Subject: Re: [PATCH v5] ld: Make archive member file extension comparisons case insensitive when cross compiling too
Date: Wed, 24 Aug 2022 13:46:04 +0300 (EEST) [thread overview]
Message-ID: <9b767c3b-2690-863c-e7dd-17fa477ee482@martin.st> (raw)
Message-ID: <20220824104604.U0WtRWh8bQ_-mZDN105wfl5eINH7iwM5g4lvhBHJxUk@z> (raw)
In-Reply-To: <de4513b6-a44a-8f8d-aef3-4504853849a0@suse.com>
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
next prev parent reply other threads:[~2022-08-24 10:46 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-23 13:06 [PATCH v2] ld: Make library member file suffix " Martin Storsjö
2022-08-23 14:01 ` Nick Clifton
2022-08-23 14:19 ` Martin Storsjö
2022-08-23 14:23 ` Jan Beulich
2022-08-23 21:11 ` [PATCH v3] ld: Make archive member file extension " Martin Storsjö
2022-08-24 6:38 ` Jan Beulich
2022-08-24 8:23 ` [PATCH v4] " Martin Storsjö
2022-08-24 9:48 ` Nick Clifton
2022-08-24 10:03 ` Martin Storsjö
2022-08-24 10:04 ` [PATCH v5] " Martin Storsjö
2022-08-24 10:29 ` Jan Beulich
2022-08-24 10:46 ` Martin Storsjö [this message]
2022-08-24 10:46 ` Martin Storsjö
2022-08-24 10:47 ` [PATCH v6] " Martin Storsjö
2022-08-24 11:17 ` Jan Beulich
2022-08-24 12:25 ` [PATCH v7] " Martin Storsjö
2022-08-24 12:39 ` Jan Beulich
2022-08-24 12:56 ` Nick Clifton
2022-08-24 20:23 ` Martin Storsjö
2022-08-25 6:53 ` [PATCH v4] " Martin Storsjö
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9b767c3b-2690-863c-e7dd-17fa477ee482@martin.st \
--to=martin@martin.st \
--cc=binutils@sourceware.org \
--cc=jbeulich@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).