Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection

[Dan Carpenter]

On Thu, Oct 19, 2017 at 01:56:32PM +0200, Michal Suchánek wrote:
> 
> I think a single cleanup section is better than many labels that just
> avoid a single null check.
>

I am not a big advocate of churn, but one err style error handling is
really bug prone.

I'm dealing with static analysis so most of the bugs I see are error
handling bugs.  That's because error handling is hard to test but easy
for static analysis.  One err style error handling is the worst because
you get things like:

fail:
	kfree(foo->bar);
	kfree(foo);

Oops, foo->bar is a NULL dereference.  And generally, it's a bad thing
to free things that haven't been allocated so, for example, I see
refcounting bugs in error handling paths as well where we decrement
something that wasn't incremented.  Freeing everything is more
complicated than just freeing one specific thing the way standard kernel
error handling works.

> As long as you can tell easily which resources were already allocated
> and need to be freed it is saner to keep only one cleanup section.
> 

Sure, if the function is simple and short then the error handling is
normally simple and short.  This is true for any style of error
handling.

> If the code doing the allocation is changed in the future the single
> cleanup can stay whereas multiple labels have to be rewritten again.

No, they don't unless you choose bad label names.  Perhaps numbered
labels?  We don't get a lot of those in the kernel any more.  Label
name should be based on what the label does.  Often I see bad label
names like generic labels:

	foo = kmalloc();
	if (!foo)
		goto out;

What is out going to do?  Another common anti-pattern is come-from
labels:

	foo = kmalloc();
	if (!foo)
		goto kmalloc_failed;

Obviously, we can see from the if statement that the alloc failed and
you *just* know the next line is going to be is going to be:

	if (invalid)
		goto kmalloc_failed;

Which is wrong because kmalloc didn't fail...  But if the label name is
based on what it does then, when you add or a remove an allocation, you
just have to edit the one thing.  It's very simple:

+	foo = new_alloc();
+	if (!foo)
+		return -ENOMEM;
+
 	bar = old_alloc();
-	if (!bar)
-		return -ENOMEM;
+	if (!bar) {
+		ret -ENOMEM;
+		goto free_foo;

[ snip ]

 free_whatever:
 	free(whatever);
+free_foo:
+	free(foo);

 return ret;

> 
> Also just changing this just for the sake of code style does not seem
> worth it whatever style you prefer.

True.

regards,
dan carpenter