Time for a code audit?

Dan Carpenter dan.carpenter at oracle.com
Tue Feb 16 06:23:23 UTC 2016


On Mon, Feb 15, 2016 at 05:35:01PM +0000, Sell, Timothy C wrote:
> Re spaghetti:
> 
> I certainly agree that most uses of 'goto' are evil.
> But I've found that when 'goto' is used to provide a single common
> exit-point for a function (e.g., "goto away"), it can lead to code
> that is much more readable and much less fragile than the alternative
> of sprinkling 'return's at various points within the function,
> especially when different clean-up actions are needed prior to each
> of those 'return's.
> 
> For me, knowing that there is exactly one place where a function can
> return (i.e., at the end), and exactly one place where we perform
> resource clean-ups (i.e., at the 'away' label) makes it easier to write
> correct code, and I think easier to understand.  'away' essentially
> fills a destructor-like role.
> 
> I don't consider such a use of 'goto' as 'spaghetti code'.
> 
> I agree that in trivial functions, this strategy would be overkill,
> and I suspect this may indeed be what is at the core of your
> argument against it.

I've written a lot about error handling.

https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ

Do everything labels are the most error prone way of doing error
handling.  I discovered this because I look at a lot of error handling
code but it makes sense because doing everything is more complicated
than doing one thing.

I have also looked at does one err style error handling prevent future
bugs.  For example, when people introduce a new return -ENOMEM; without
dropping the lock, does it matter if there is an out: label at the
bottom of the function and basically it does not.  The people who don't
care about locks don't care about common exit paths either.

This style also causes future bugs when we change from a NULL on error
kmalloc(), copy_from_user() to an error pointer function like
kmemdup_user().  If you use canonical unwinding then you don't have that
problem because you don't free things which have not been allocated.

One err style causes bugs in the present and it causes bugs in the
future.  And empirically, historically it doesn't prevented people
from adding new returns in the middle of functions.

> >    781          fix_vbus_dev_info(dev);
> >    782          up(&dev->visordriver_callback_lock);
> >    783          rc = 0;
> >    784  away:
> >    785          if (rc != 0)
> >    786                  put_device(&dev->device);
> >    787          return rc;
> >    788  }

What makes this spaghetti code is that we twist the error paths and the
success paths together.  I put it in my email because it was ugly, but
looking at it now I see it's also buggy.  It's hard to argue this
prevents future bugs when it is buggy right now as-is.

Of course, that's a sample size of one.  Let's look at the rest of
the file.

visorbus_match(), it's impossible to tell if the error codes are
intentional which is typical for do-nothing gotos.  The comment is not
totally helpful.

   343  away:
   344          if (rc < 0) {
   345                  kfree(myattr);
   346                  myattr = NULL;
   347                  dev->devnodes[slot].attr = NULL;
   348          }
   349          return rc;
   350  }

Memory corruption.  Gar, Smatch should have caught this.

create_visor_device() has incomplete error handling.

  1460          rc = 0;
  1461  away:
  1462          if (rc < 0) {
  1463                  if (notify_func)
  1464                          (*notify_func)(dev, rc);
  1465          }
  1466  }

Hm...  This one is tricky.  rc is -1.  notify_func is either
visorchipset_device_pause_response() or device_resume_response().  I'm
a bit confused about ->pending_msg_hdr and how that is protected from
races.  We do "msg->hdr.completion_status = (u32)(-response);" where
response is -1.  It's hard to tell what's going on here.  I think
normally we pass positive error codes to response like
CONTROLVM_RESP_ERROR_DEVICE_UDEV_TIMEOUT (1400)?

That's the end of the file.  So most of the time if the away label does
something more complicated than printing a message and and returning
it's probably buggy.

regards,
dan carpenter



More information about the devel mailing list