inner/outer RAII in C

example.c

#include <errno.h>
#include <stdio.h>

// We want a function that creates a couple files. Here's the naive approach:

int hardcoded_cleanup(const char *path1, const char *path2) {
  FILE *file1 = fopen(path1, "w");
  if (file1 == NULL) {
    return errno;
  }
  FILE *file2 = fopen(path2, "w");
  if (file2 == NULL) {
    int ret = errno;
    fclose(file1);
    return ret;
  }
  int n = fwrite("foo", 1, 3, file1);
  if (n != 3) {
    int ret = errno;
    fclose(file1);
    fclose(file2);
    return ret;
  }
  n = fwrite("bar", 1, 3, file2);
  if (n != 3) {
    int ret = errno;
    fclose(file1);
    fclose(file2);
    return ret;
  }
  if (fclose(file1) == EOF) {
    int ret = errno;
    fclose(file2);
    return ret;
  }
  if (fclose(file2) == EOF) {
    return errno;
  }
  return 0;
}

// This sucks. We're copying lots of cleanup code, and the cleanup code is
// subtly different in different places, which makes it vulnerable to copy-paste
// mistakes. A common solution to this is to unify the error handling at the
// bottom and use gotos:

int goto_cleanup(const char *path1, const char *path2) {
  int ret = 0;
  FILE *file1 = NULL;
  FILE *file2 = NULL;
  file1 = fopen(path1, "w");
  if (file1 == NULL) {
    ret = errno;
    goto cleanup;
  }
  file2 = fopen(path2, "w");
  if (file2 == NULL) {
    ret = errno;
    goto cleanup;
  }
  int n = fwrite("foo", 1, 3, file1);
  if (n != 3) {
    ret = errno;
    goto cleanup;
  }
  n = fwrite("bar", 1, 3, file2);
  if (n != 3) {
    ret = errno;
    goto cleanup;
  }
  int close_ret = fclose(file1);
  file1 = NULL;
  if (close_ret == EOF) {
    ret = errno;
    goto cleanup;
  }
  close_ret = fclose(file2);
  file2 = NULL;
  if (close_ret == EOF) {
    ret = errno;
    goto cleanup;
  }
cleanup:
  if (file1 != NULL) {
    fclose(file1);
  }
  if (file2 != NULL) {
    fclose(file2);
  }
  return ret;
}

// This is somewhat better. The error handling is more consistent, and we're
// less likely to make mistakes. But it's got a few downsides:
//
// - Goto is problematic, and we'd rather ban it.
// - If we forget to NULL-initialize all our variables at the top, the cleanup
//   block will operate on uninitialized data, probably in an error case that's
//   hard to test. There's no compiler warning for that.
// - Going to cleanup without assigning to `ret` is almost certainly a bug, but
//   there's no compiler warning for that either. This is easier to catch when
//   every error handling branch looks the same, as in this example, but that's
//   not always the case.
//
// Here's an alternative inner/outer resource management pattern that I prefer:

int _inner_function(const char *path1, const char *path2, FILE **file1, FILE **file2) {
  // This function is private. It doesn't do any cleanup.
  *file1 = fopen(path1, "w");
  if (*file1 == NULL) {
    return errno;
  }
  *file2 = fopen(path2, "w");
  if (*file2 == NULL) {
    return errno;
  }
  int n = fwrite("foo", 1, 3, *file1);
  if (n != 3) {
    return errno;
  }
  n = fwrite("bar", 1, 3, *file2);
  if (n != 3) {
    return errno;
  }
  int close_ret = fclose(*file1);
  *file1 = NULL;
  if (close_ret == EOF) {
    return errno;
  }
  close_ret = fclose(*file2);
  *file2 = NULL;
  if (close_ret == EOF) {
    return errno;
  }
  return 0;
}

int outer_function(const char *path1, const char *path2) {
  // This function is public. It has no early returns.
  FILE *file1 = NULL;
  FILE *file2 = NULL;
  int ret = _inner_function(path1, path2, &file1, &file2);
  if (file1 != NULL) {
    fclose(file1);
  }
  if (file2 != NULL) {
    fclose(file2);
  }
  return ret;
}

// The inner/outer approach above has several benefits:
//
// - No gotos.
// - Resource declarations are right next to cleanup code, so it's easier to
//   see that all resources are NULL-initialized and that we can't call any
//   free/close functions on an uninitialized pointer.
// - The compiler type-checks all of our error returns. No forgotten error
//   codes.
// - The "interesting" code in the inner function is tighter, with fewer
//   copy-pasted lines.
//
// On the other hand, it has a couple downsides:
//
// - Writing two functions instead of one is annoying.
// - It's easy to forget to dereference a pointer in one of the NULL checks, or
//   with functions like memcpy that take void*. The compiler doesn't catch
//   these mistakes, and the result can be a silent error.