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