I told you handing errors was error prone

Wednesday, Debtember 02, 2009

From
Mark Grosberg <XXXXXXXXXXXXXXXXX>
To
sean@conman.org
Subject
Blog post.
Date
Wed, 2 Dec 2009 15:59:55 -0500 (EST)
I find it even more amusing that you didn't get the error handling right in the create_socket() on your current blog post.
Notice that you leak the socket and/or memory in the error cases. I guess it really is hard to handle errors. ;-)
Sorry, I just had to take this cheap shot!
-MYG

Heh.

Yup, I blew it again for demonstration purposes.

The code I posted yesterday was actually pulled from a current project where the create_socket() is only called during initialization and if it fails, the program exits. Since I'm on a Unix system, the “lost” resources like memory and sockets are automatically reclaimed. Not all operating systems are nice like this.

There are a few ways to fix this. One, is to use a langauge that handles such details automatically with garbage collection, but I'm using C so that's not an option. The second one is to add cleanup code at each point we exit, but using that we end up with code that looks like:

  /* ... */

  if fcntl(listen->sock,F_GETFL,0) == -1)
  {
    perror("fcntl(GETFL)");
    close(listen->socket); 
    free(listen);
    return -1;
  }
   
  if (fcntl(listen->sock,F_SETFL,rc | O_NONBLOCK) == -1)
  {
    perror("fcntl(SETFL)");
    close(listen->socket); 
    free(listen);
    return -1;   
  }
   
  if (bind(listen->sock,paddr,saddr) == -1)
  {
    perror("bind()");
    close(listen->socket);
    free(listen);
    return -1;   
  }

  /* ... */

Lots of duplicated code and the more complex the routine, the more complex the cleanup and potential to leak memory (or other resources like files and network connections). The other option looks like:

  /* ... */

  if fcntl(listen->sock,F_GETFL,0) == -1)
  {
    perror("fcntl(GETFL)");
    goto create_socket_cleanup;
  }
  
  if (fcntl(listen->sock,F_SETFL,rc | O_NONBLOCK) == -1)
  {
    perror("fcntl(SETFL)");
    goto create_socket_cleanup;
  }

  if (bind(listen->sock,paddr,saddr) == -1)
  {
    perror("bind()");
    goto create_socket_cleanup;
  }

  /* rest of code */

  return listen->sock; /* everything is okay */

create_socket_cleanup:
  close(listen->sock);
create_socket_cleanup_mem:
  free(listen);
  return -1;
}

This uses the dreaded goto construct, but is one of the few places that it's deemed “okay” to use goto, for cleaning up errors. No code duplication, but you need to make sure you cleanup (or unwind, or whatever) in reverse order.

So yeah, error handling … maddening.

I still wish there was a better way …

The Boston Diaries

Wednesday, Debtember 02, 2009

I told you handing errors was error prone

Obligatory Picture

Obligatory Contact Info

Obligatory Feeds

Obligatory Links

Obligatory Miscellaneous