Freeing Memory Before Panic Considered Harmful?

(I will admit I dislike ‘x Considered Harmful’ essays, so take the title as a tongue-in-cheek.)

There’s this common adage going around at UofG’s C courses (ahem, CIS*2750) that when your program encounters an error and quits (i.e. “panics”), it should free all of its buffers before exiting. I think this is a colossal waste of time. The University of Guelph’s C courses worry too much about being pedantic and “correct” that, in my opinion, it wastes a lot of curriculum time.

Most sane operating systems (at least the ones used at the University of Guelph SoCS infrastructure) will free all of buffers in a given program before exiting. Typically such memory is reported by valgrind as

​ ​ ​ ​ still reachable: 151, 526 bytes in 1,256

in the LEAK SUMMARY:. Seeing how you’re not using the memory you allocated anyway (especially in the case of abnormal program termination), what difference does it make whether or not you free those buffers? You’re just introducing:

• More code/busywork to maintain
• State/scope issues with keeping track of what memory exists
• A higher chance of developer error in freeing said buffers (e.g. if a student were to terminate their 2750 app because of an invalid .SVG file, attempts to free their buffers and then accidentally double-frees a chunk of memory, causing an actual program crash and then getting screwed over by the TA as a result). Ideally, you want to do as little processing as possible upon an abnormal program exit—try not to contaminate your debugging trace!

Actually, about that second point, I remember in my CIS*2750 class, Denis (err, Prof. Nikitenko) put the cohort into cognitive dissonance, supposing to us that it “wouldn’t be a bad idea to use a goto statement to keep track of error handling). And indeed! Panic gotos are common in several glibc functions. A common pattern is to place some panic-handling code below the return statement, or after whatever code that isn’t executed in regular operation:

        if (new_fd == -1)
            goto fail;

        /* Make sure the desired file descriptor is used.  */
        if (new_fd != action->action.open_action.fd)
        {
            if (__dup2 (new_fd, action->action.open_action.fd)
            != action->action.open_action.fd)
              goto fail;

            if (__close_nocancel (new_fd) != 0)
              goto fail;
        }
    }
    break;

    case spawn_do_dup2:
        if (__dup2 (action->action.dup2_action.fd,
                    action->action.dup2_action.newfd)
           != action->action.dup2_action.newfd)
           goto fail;
    break; 
}

...

    /* This is compatibility function required to enable posix_spawn run
       script without shebang definition for older posix_spawn versions
       (2.15).  */
    maybe_script_execute (args);

fail:
    /* errno should have an appropriate non-zero value; otherwise,
       there's a bug in glibc or the kernel.  For lack of an error code
       (EINTERNALBUG) describing that, use ECHILD.  Another option would
       be to set args->err to some negative sentinel and have the parent
       abort(), but that seems needlessly harsh.  */
    ret = errno ? : ECHILD;
    if (ret)
        /* Since sizeof errno < PIPE_BUF, the write is atomic. */
        while (__write_nocancel (args->pipe[1], &ret, sizeof (ret)) < 0);

    _exit (SPAWN_ERROR);
}

if (name != NULL)
{
    if (*buf == '\0')
	    if (pts_name (master, &buf, sizeof (_buf)))
		    goto on_error;

    strcpy (name, buf);
}

ret = 0;

on_error:
	if (ret == -1) {
		close (master);
		if (slave != -1)
			close (slave);
	}

	if (buf != _buf)
		free (buf);

return ret;

      if (__glibc_unlikely (((~(hi ^ (res - hi)) & (res ^ hi)) < 0)))
	      goto overflow;
      return res;
  }
  else
  {
      if (xh > 0.0)
	      hi = __LONG_LONG_MAX__;
      else if (xh < 0.0)
	      hi = -__LONG_LONG_MAX__ - 1;
      else
	      /* Nan */
	      hi = 0;
  }

overflow:
#ifdef FE_INVALID
    feraiseexcept (FE_INVALID);
#endif
    return hi;
}

	            else if (!S_ISDIR (st.st_mode) && *end != '\0')
	            {
	                __set_errno (ENOTDIR);
	                goto error;
	            }
	        }
        }
    if (dest > rpath + 1 && dest[-1] == '/')
      --dest;
    *dest = '\0';

    assert (resolved == NULL || resolved == rpath);
    return rpath;

error:
    assert (resolved == NULL || resolved == rpath);
    if (resolved == NULL)
      free (rpath);
    return NULL;
}

Several people I knew in the cohort immediately started debating with one another on whether it was sound to use a goto. This fear stemmed from a long-running policy in earlier courses (such as CIS*1500) where using a single goto was grounds for an instant 0 on any assignment. This practice I also considered abhorrent. Nonetheless, some of the proposed solutions that people undertook were:

• Using the <stdarg.h> library header and creating a variadic function that would free any number of buffers
  • This is the same header used by functions like printf and scanf that allow a dynamic number of arguments. The issue with this is is that it works for single buffers, but what about buffers that pointed to other buffers (e.g. a struct that had internal pointers it had to free itself. Also, what about nested buffers?)
• Creating a variadic macro (#define and __VAR_ARGS__, anyone?)
  • Same thing as the above, but without type-checking. Yuck.
• Artificially keeping track of all of the buffers being held and whether they were allocated at the time of panic (this is typically how you’ll see error-freeing being implemented in glibc as shown in the last example.
  • Now, if you’re a conformant programmer who likes making small functions, this isn’t all that bad. But in my experience, students in CIS*2750 would have to develop an aggregate “creator” function (e.g. createCalendar, createSVG, createGEDCOM, etc.) and given how we were shoehorned into Prof. Nikitenko’s function definitions, this would usually result in maintaining a vast amount of flags. Again, messy. This would also bring people back into the days of ANSI C where you had to put all of your declarations before your actual logic. This practice (typically introduced in CIS*2500) made reading program scope extremely confusing, and would result in a lot of duplicate code.

As you can see, all these solutions resulted in many more headaches over a nonexistent problem. Some people even argue that freeing memory at the end of normal execution isn’t needed either. These folks claim that there is no tangible benefit (or difference) in freeing memory right before exit vs. just letting the operating system reclaim it anyway. You can see this with a few common Linux utilities anyway (g++ and make as shown below):

==17030== Memcheck, a memory error detector
==17030== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==17030== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==17030== Command: g++ main.cpp
==17030==
==17030==
==17030== HEAP SUMMARY:
==17030==     in use at exit: 181,041 bytes in 94 blocks
==17030==   total heap usage: 391 allocs, 297 frees, 235,066 bytes allocated
==17030==
==17030== LEAK SUMMARY:
==17030==    definitely lost: 5,463 bytes in 26 blocks
==17030==    indirectly lost: 82 bytes in 5 blocks
==17030==      possibly lost: 0 bytes in 0 blocks
==17030==    still reachable: 175,496 bytes in 63 blocks
==17030==         suppressed: 0 bytes in 0 blocks
==17030== Rerun with --leak-check=full to see details of leaked memory
==17030==
==17030== For counts of detected and suppressed errors, rerun with: -v
==17030== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

==17000== Memcheck, a memory error detector
==17000== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==17000== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==17000== Command: make
==17000==
clang -Wall -Wpedantic -Wextra -ggdb3 -Iinclude -Iseethe  bin/test_sagittarius.o bin/sagittarius.o -lm -o bin/test_sagittarius
==17000==
==17000== HEAP SUMMARY:
==17000==     in use at exit: 151,326 bytes in 1,256 blocks
==17000==   total heap usage: 1,853 allocs, 597 frees, 410,410 bytes allocated
==17000==
==17000== LEAK SUMMARY:
==17000==    definitely lost: 0 bytes in 0 blocks
==17000==    indirectly lost: 0 bytes in 0 blocks
==17000==      possibly lost: 0 bytes in 0 blocks
==17000==    still reachable: 151,326 bytes in 1,256 blocks
==17000==         suppressed: 0 bytes in 0 blocks
==17000== Rerun with --leak-check=full to see details of leaked memory
==17000==
==17000== For counts of detected and suppressed errors, rerun with: -v
==17000== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

The valgrind FAQ goes as far as support this claim as well:

“still reachable” means your program is probably ok — it didn’t free some memory it could have. This is quite common and often reasonable. Don’t use --show-reachable=yes if you don’t want to see these reports.

valgrind FAQ

I’m entirely aware that “freeing memory before normal exit” is a bigger holy war than “freeing memory before abnormal exit”, so I won’t touch that one. But read here if you’re curious about that crusade.

In addition to the points above, free‘ing memory isn’t a cheap operation. Re-affixing the program break and having the system re-do memory book-keeping while your program crashes should be the least of either the user or the developer’s worries when your program is figuratively burning down. Only a fool would justify compromising performance just for the sake of a “clean exit”. What’s next? Trying to capture a SIGINT signal using a signal handling function so that your parser frees all memory whenever the TA presses CTRL+C? And then what? What about the stronger CTRL+Z? (Trick question—you can’t capture SIGSTOP for safety/kernel reasons).

Trying to capture every possible avenue of user error is simply an exercise in entropy.

Tl;dr: close your FILE pointers and (mayyybe) flush your buffers when you panic; don’t bother trying to close the door behind you quietly if you know your house is burning down.