Re: [PATCH] DTC: Fix unprotected file name member access.

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

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

On Mon, Jul 11, 2016 at 08:22:06AM +0200, Jean-Christophe DUBOIS wrote:
> Hello David,
> 
> Le 11/07/2016 03:30, David Gibson a écrit :
> > On Mon, Jul 11, 2016 at 12:17:08AM +0200, Jean-Christophe Dubois wrote:
> > > Signed-off-by: Jean-Christophe Dubois <jcd-WBS85hRCVJbxB9160cZjhg@public.gmane.org>
> > A patch like this really needs a commit message describing when the
> > situation it's guarding against can occur.  In this case when can
> > pos->file be non-NULL, but pos->file->name == NULL.
> I stumbled on these issues while running some code checkers on previous
> versions of the DTC compiler.
> 
> In general it's seems like a good idea for a function to protect itself
> against bad use.

Not necessarily.  Such checks can mask the effects of a bug elsewhere,
meaning you get subtler and worse (or at least harder to debug)
problems down the track.

If you must check, use an assert() rather than silently carrying on as
if the arguments were valid.  I detest silently hiding error
conditions or bugs.

> I have not looked at  the DTC source tree to check if such
> bad use could happen at present (and I don't know about future) but I do
> feel safer with these kinds of checks in place.

I absolutely do not like putting checks for the sake of them without
understanding the context in which they could (or could not) occur.

> > It's not exactly high priority, since srcpos_dump() has no current
> > users AFAICT.
> srcpos_dump() doesn't seem to be used for now but the forced cast of the
> "file" member as a (char *) is a bit disturbing in the source code. The
> first user might be surprised of the output.

Well, no, we shold just get rid of it, since there are no current or
intending users.  <...> I've now done that.

> srcpos_string() seems to be used mainly in error case (srcpos_verror,
> srcpos_error, ...) which might be considered as unimportant (?) but still, I
> feel safer with the check (a more meaningful output?).

There's a huge difference between used in error cases and not used at all.

> Now it is certainly not high priority (everything is working OK in the
> normal case) but it is cheap to fix.
> 
> Regards
> 
> JC
> 
> > 
> > > ---
> > >   srcpos.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/srcpos.c b/srcpos.c
> > > index af7fb3c..927ac54 100644
> > > --- a/srcpos.c
> > > +++ b/srcpos.c
> > > @@ -252,7 +252,7 @@ void
> > >   srcpos_dump(struct srcpos *pos)
> > >   {
> > >   	printf("file        : \"%s\"\n",
> > > -	       pos->file ? (char *) pos->file : "<no file>");
> > > +	       (pos->file && pos->file->name) ? pos->file->name : "<no file>");
> > >   	printf("first_line  : %d\n", pos->first_line);
> > >   	printf("first_column: %d\n", pos->first_column);
> > >   	printf("last_line   : %d\n", pos->last_line);
> > > @@ -267,7 +267,7 @@ srcpos_string(struct srcpos *pos)
> > >   	const char *fname = "<no-file>";
> > >   	char *pos_str;
> > > -	if (pos)
> > > +	if (pos->file && pos->file->name)
> > >   		fname = pos->file->name;
> 

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