Re: [RFC PATCH v6 1/3] dtc: protect against null pointer dereference in srcpos_string()

[Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 10/5/2015 9:10 PM, David Gibson wrote:
> On Fri, Oct 02, 2015 at 09:49:08PM -0700, Frank Rowand wrote:
>> From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
>>
>> Check for NULL pos before dereferencing it in srcpos_string().
>>
>> Signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
>> ---
>>  srcpos.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> Index: b/srcpos.c
>> ===================================================================
>> --- a/srcpos.c
>> +++ b/srcpos.c
>> @@ -268,11 +268,13 @@ srcpos_string(struct srcpos *pos)
>>  	char *pos_str;
>>  	int rc;
>>  
>> -	if (pos)
>> +	if (pos && pos->file)
>>  		fname = pos->file->name;
>>  
>>  
>> -	if (pos->first_line != pos->last_line)
>> +	if (!pos)
>> +		rc = asprintf(&pos_str, "%s:<no-line>", fname);
>> +	else if (pos->first_line != pos->last_line)
> 
> This logic still seems backwards to me.  I'd really prefer the !pos
> check to go first, then !pos->file, then the normal case.
> 

Checking !pos first results in either an early return, a goto,
or more deeply nesting the

   if (pos->first_line != pos->last_line)
      asprintf(...)
   else if (...)
      asprintf(...)
   else
      rc = asprintf(...)

all of which seemed worse.  In the next version I'll change it to:

char *
srcpos_string(struct srcpos *pos)
{
        const char *fname = "<no-file>";
        char *pos_str;
        int rc;

        if (!pos) {
                rc = asprintf(&pos_str, "%s:<no-line>", fname);
                goto out;
        } else if (pos->file) {
                fname = pos->file->name;
        }

        if (pos->first_line != pos->last_line)
                rc = asprintf(&pos_str, "%s:%d.%d-%d.%d", fname,
                              pos->first_line, pos->first_column,
                              pos->last_line, pos->last_column);
        else if (pos->first_column != pos->last_column)
                rc = asprintf(&pos_str, "%s:%d.%d-%d", fname,
                              pos->first_line, pos->first_column,
                              pos->last_column);
        else
                rc = asprintf(&pos_str, "%s:%d.%d", fname,
                              pos->first_line, pos->first_column);

out:
        if (rc == -1)
                die("Couldn't allocate in srcpos string");

        return pos_str;
}