[PATCH] gold: fix crash for empty file

[PATCH] gold: fix crash for empty file

[Martin Liška]

Hi.

The patch fixes a crash I noticed and explained in the linked PR.

Patch survives regression tests.

Ready to be installed?
Thanks,
Martin

gold/ChangeLog:

	PR 28585
	* symtab.cc (Symbol_table::lookup): Return NULL for NULL
	argument.
---
  gold/symtab.cc | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/gold/symtab.cc b/gold/symtab.cc
index 5a21ddc8cc2..ed6b5434592 100644
--- a/gold/symtab.cc
+++ b/gold/symtab.cc
@@ -701,6 +701,8 @@ Symbol_table::resolve_forwards(const Symbol* from) const
  Symbol*
  Symbol_table::lookup(const char* name, const char* version) const
  {
+  if (name == NULL)
+    return NULL;
    Stringpool::Key name_key;
    name = this->namepool_.find(name, &name_key);
    if (name == NULL)
-- 
2.33.1

Re: [PATCH] gold: fix crash for empty file

[Martin Liška]

PING^1

On 11/19/21 15:01, Martin Liška wrote:
> Hi.
> 
> The patch fixes a crash I noticed and explained in the linked PR.
> 
> Patch survives regression tests.
> 
> Ready to be installed?
> Thanks,
> Martin
> 
> gold/ChangeLog:
> 
>      PR 28585
>      * symtab.cc (Symbol_table::lookup): Return NULL for NULL
>      argument.
> ---
>   gold/symtab.cc | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/gold/symtab.cc b/gold/symtab.cc
> index 5a21ddc8cc2..ed6b5434592 100644
> --- a/gold/symtab.cc
> +++ b/gold/symtab.cc
> @@ -701,6 +701,8 @@ Symbol_table::resolve_forwards(const Symbol* from) const
>   Symbol*
>   Symbol_table::lookup(const char* name, const char* version) const
>   {
> +  if (name == NULL)
> +    return NULL;
>     Stringpool::Key name_key;
>     name = this->namepool_.find(name, &name_key);
>     if (name == NULL)

Re: [PATCH] gold: fix crash for empty file

[Martin Liška]

PING^2

On 11/29/21 10:00, Martin Liška wrote:
> PING^1
> 
> On 11/19/21 15:01, Martin Liška wrote:
>> Hi.
>>
>> The patch fixes a crash I noticed and explained in the linked PR.
>>
>> Patch survives regression tests.
>>
>> Ready to be installed?
>> Thanks,
>> Martin
>>
>> gold/ChangeLog:
>>
>>      PR 28585
>>      * symtab.cc (Symbol_table::lookup): Return NULL for NULL
>>      argument.
>> ---
>>   gold/symtab.cc | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/gold/symtab.cc b/gold/symtab.cc
>> index 5a21ddc8cc2..ed6b5434592 100644
>> --- a/gold/symtab.cc
>> +++ b/gold/symtab.cc
>> @@ -701,6 +701,8 @@ Symbol_table::resolve_forwards(const Symbol* from) const
>>   Symbol*
>>   Symbol_table::lookup(const char* name, const char* version) const
>>   {
>> +  if (name == NULL)
>> +    return NULL;
>>     Stringpool::Key name_key;
>>     name = this->namepool_.find(name, &name_key);
>>     if (name == NULL)
> 

Re: [PATCH] gold: fix crash for empty file

[Cary Coutant]

> >> @@ -701,6 +701,8 @@ Symbol_table::resolve_forwards(const Symbol* from) const
> >>   Symbol*
> >>   Symbol_table::lookup(const char* name, const char* version) const
> >>   {
> >> +  if (name == NULL)
> >> +    return NULL;

I'm more concerned with why a NULL name got passed to lookup in the
first place. In this test case, it's because parameters->entry() is
returning NULL, which in turn is because no (valid) object files have
been named, so a target hasn't been determined. I'd be more
comfortable with maintaining the requirement of passing a non-null
name to Symbol_table::lookup() (perhaps even adding an assert to that
effect), and then fixing Plugin_hook::run() to check for NULL before
calling lookup.

-cary

Re: [PATCH] gold: fix crash for empty file

[Martin Liška]

On 1/5/22 18:35, Cary Coutant wrote:
>>>> @@ -701,6 +701,8 @@ Symbol_table::resolve_forwards(const Symbol* from) const
>>>>    Symbol*
>>>>    Symbol_table::lookup(const char* name, const char* version) const
>>>>    {
>>>> +  if (name == NULL)
>>>> +    return NULL;
> 
> I'm more concerned with why a NULL name got passed to lookup in the
> first place. In this test case, it's because parameters->entry() is
> returning NULL, which in turn is because no (valid) object files have
> been named, so a target hasn't been determined. I'd be more
> comfortable with maintaining the requirement of passing a non-null
> name to Symbol_table::lookup() (perhaps even adding an assert to that
> effect), and then fixing Plugin_hook::run() to check for NULL before
> calling lookup.

Sure, I must confess I don't know gold enough. Thus, leaving that to you.

Cheers,
Martin

> 
> -cary