Re: [PATCH] srcpos.c: Fix dereference after null check

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3700 bytes --]

On Tue, Feb 07, 2017 at 06:08:14PM +0100, Jean-Christophe Dubois wrote:
> When running coverity on dtc source code the following error is reported.
> 
> =========================================================================
> 250 srcpos_string(struct srcpos *pos)
> 251{
> 252        const char *fname = "<no-file>";
> 253        char *pos_str;
> 254
>    1. Condition pos, taking false branch.
>    2. var_compare_op: Comparing pos to null implies that pos might be null.
> 255        if (pos)
> 256                fname = pos->file->name;
> 257
> 258
>    CID 1026108 (#1 of 1): Dereference after null check (FORWARD_NULL)3. var_deref_op: Dereferencing null pointer pos.
> 259        if (pos->first_line != pos->last_line)
> 
> =======================================================================
> 
> Fixed the srcpos_string() function assuming the pos argument could be NULL.
> 
> Fixed the fname assignement checking the name was indeed set in the
> provided pos structure.
> 
> As srcpos_string() can now return NULL, fixed srcpos_verror() to handle this
> situation.

Actually, on looking at this a bit further, it looks like the post ==
NULL case just can't happen.  I've instead made a change to just
remove the pos == NULL check from srcpos_string().


> 
> Signed-off-by: Jean-Christophe Dubois <jcd-WBS85hRCVJbxB9160cZjhg@public.gmane.org>
> ---
>  srcpos.c | 44 ++++++++++++++++++++++++++------------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/srcpos.c b/srcpos.c
> index aa3aad0..cec790c 100644
> --- a/srcpos.c
> +++ b/srcpos.c
> @@ -249,24 +249,28 @@ srcpos_copy(struct srcpos *pos)
>  char *
>  srcpos_string(struct srcpos *pos)
>  {
> -	const char *fname = "<no-file>";
> -	char *pos_str;
> +	char *pos_str = NULL;
>  
> -	if (pos)
> -		fname = pos->file->name;
> +	if (pos) {
> +		const char *fname = "<no-file>";
>  
> +		if (pos->file && pos->file->name) {
> +			fname = pos->file->name;
> +		}
>  
> -	if (pos->first_line != pos->last_line)
> -		xasprintf(&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)
> -		xasprintf(&pos_str, "%s:%d.%d-%d", fname,
> -			  pos->first_line, pos->first_column,
> -			  pos->last_column);
> -	else
> -		xasprintf(&pos_str, "%s:%d.%d", fname,
> -			  pos->first_line, pos->first_column);
> +		if (pos->first_line != pos->last_line) {
> +			xasprintf(&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) {
> +			xasprintf(&pos_str, "%s:%d.%d-%d", fname,
> +				  pos->first_line, pos->first_column,
> +				  pos->last_column);
> +		} else {
> +			xasprintf(&pos_str, "%s:%d.%d", fname,
> +				  pos->first_line, pos->first_column);
> +		}
> +	}
>  
>  	return pos_str;
>  }
> @@ -278,11 +282,15 @@ void srcpos_verror(struct srcpos *pos, const char *prefix,
>  
>  	srcstr = srcpos_string(pos);
>  
> -	fprintf(stderr, "%s: %s ", prefix, srcstr);
> +	if (srcstr) {
> +		fprintf(stderr, "%s: %s ", prefix, srcstr);
> +		free(srcstr);
> +	} else {
> +		fprintf(stderr, "%s: <unknown> ", prefix);
> +	}
> +
>  	vfprintf(stderr, fmt, va);
>  	fprintf(stderr, "\n");
> -
> -	free(srcstr);
>  }
>  
>  void srcpos_error(struct srcpos *pos, const char *prefix,

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]