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

2015-10-06 Thread David Gibson 
On Tue, Oct 06, 2015 at 12:32:20AM -0700, Frank Rowand wrote:
> 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 
> >>
> >> Check for NULL pos before dereferencing it in srcpos_string().
> >>
> >> Signed-off-by: Frank Rowand 
> >> ---
> >>  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:", 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

Early return is fine by me.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

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

2015-10-06 Thread Frank Rowand 
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 
>>
>> Check for NULL pos before dereferencing it in srcpos_string().
>>
>> Signed-off-by: Frank Rowand 
>> ---
>>  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:", 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 = "";
char *pos_str;
int rc;

if (!pos) {
rc = asprintf(&pos_str, "%s:", 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;
}

--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" 
in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-10-05 Thread David Gibson 
On Fri, Oct 02, 2015 at 09:49:08PM -0700, Frank Rowand wrote:
> From: Frank Rowand 
> 
> Check for NULL pos before dereferencing it in srcpos_string().
> 
> Signed-off-by: Frank Rowand 
> ---
>  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:", 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.

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

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

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

2015-10-02 Thread Frank Rowand 
From: Frank Rowand 

Check for NULL pos before dereferencing it in srcpos_string().

Signed-off-by: Frank Rowand 
---
 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:", fname);
+   else 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);
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" 
in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html