Re: [PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

["Giedrius Statkevičius" <giedrius.statkevicius@gmail.com>]

On Tue, 7 Apr 2015, Dan Carpenter wrote:

> You will need to update the subject to reflect the new patch.
> 
> The original code did check for kzalloc() failure but it had lots of
> checks scattered around instead nicely at the point where the memory
> was allocated.
> 

There are a lot missing too. For example in dgnc_sysfs.c there are no checks on
any _show() methods if ->channels[i] is NULL or not. And in some other places.

> The old code and the new code are both buggy though and will crash in
> dgnc_tty_uninit().  dgnc_found_board() does "One Err" style error
> handling so it's obviously buggy like the underside of a rock.
> https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ
> 
> It's becoming a difficult thing to fix this because every time we look
> there are more things which don't make sense.
> 
> I believe that if you do:
> 
> > +err_free_channels:
> > +	for (i = i - 1; i >= 0; --i) {
> > +		kfree(brd->channels[i]);
> 		brd->channels[i] = NULL;
> 	}
> > +	return -ENOMEM;
> >  }
> 
> And add some NULL checks in dgnc_tty_uninit() to see if ->channels[i] is
> NULL before doing:
> 
> 	dgnc_remove_tty_sysfs(brd->channels[i]->ch_tun.un_sysfs);
> 
> and
> 	dgnc_remove_tty_sysfs(brd->channels[i]->ch_pun.un_sysfs);
> 
> Then it will fix the bug.
> 

Missed this. Yep, I agree. 

> Do these in a separate patch.  I'm looking for ways we can make this
> patch minimal.  Deleting the comments and the NULL check in
> dgnc_tty_init() is essential for the patch because otherwise the cleanup
> doesn't make sense.

Well, the point of the patch is to put alloc and checks in one place to make the
code less error and bug prone and fix some of bugs where ->channels[i] isn't
checked. Ok, I'll send a v5 that's split into two patches.

> 
> regards,
> dan carpenter
> 
> 

Su pagarba / Regards,
Giedrius