3

I have a simple program which uses select and stuff like that for multiplexing IO.
To interrupt the "server" process, I integrated a sig_handler which reacts on SIGINT.

Each time when memory is allocated, the containing method does the free itself or the calling method.

Using valgrind shows up, that there are some allocation aren't freed up.
Perhaps there is no need for it, but I want to learn what would be the best way to handle the signal.
It seems that the free calls aren't invoked when pressing STRG + C.
So it would be senseless to quit loops with a break condition, which was my first approach.

Is there any possibility to clean every thing up, before closing the whole program?

Thanks for any hints and advices.

CSchulz
  • 10,882
  • 11
  • 60
  • 114
  • There is no need to do this if you plan to exit. Only if you plan to continue then there is a need to ensure there is no leak. – pizza Jul 15 '12 at 02:20

3 Answers3

13

Valgrind is just a tool for finding memory leaks, not an oracle whose advice must be heeded. Making a program "Valgrind-clean" is a worthy goal but don't let it get out of hand. Ask yourself some questions about the program.

  1. Does the program need to do anything when it receives a SIGINT or SIGQUIT or whatever? Does it need to do some kind of clean shutdown? For example, a server might decide to finish handling all open requests, or at least send a shutdown message to connected clients.

  2. Does a sudden termination always leave behind certain blocks? Then you can just quash those reports from Valgrind, without having to spend extra time freeing memory that's already going to be freed.

In simple terms, there are only two reasons to call free in a program that is about to exit.

  1. If it's the easiest way to quash Valgrind messages (that is, without reading the Valgrind manual)

  2. If it makes your code simpler.

Otherwise, just don't call free during program exit, since all it will do is burn CPU cycles.

Handling SIGINT: I can think of four common ways to handle a SIGINT:

  1. Use the default handler. Highly recommended, this requires the least amount of code and is unlikely to result in any abnormal program behavior. Your program will simply exit.

  2. Use longjmp to exit immediately. This is for folk who like to ride fast motorcycles without helmets. It's like playing Russian roulette with library calls. Not recommended.

  3. Set a flag, and interrupt the main loop's pselect/ppoll. This is a pain to get right, since you have to twiddle with signal masks. You want to interrupt pselect/ppoll only, and not non-reentrant functions like malloc or free, so you have to be very careful with things like signal masks. Not recommended. You have to use pselect/ppoll instead of select/poll, because the "p" versions can set the signal mask atomically. If you use select or poll, a signal can arrive after you check the flag but before you call select/poll, this is bad.

  4. Create a pipe to communicate between the main thread and the signal handler. Always include this pipe in the call to select/poll. The signal handler simply writes one byte to the pipe, and if the main loop successfully reads one byte from the other end, then it exits cleanly. Highly recommended. You can also have the signal handler uninstall itself, so an impatient user can hit CTRL+C twice to get an immediate exit.

The two simplest and most fool-proof methods are #1 and #4.

A program that exits does not have any leaks. Only a running program can have leaks. Once the program exits, all memory is freed (so there is no leak any more).

Dietrich Epp
  • 205,541
  • 37
  • 345
  • 415
1

Here's my simple and slightly dirty solution.

#include <signal.h>

volatile bool gContinue; 

void handleCtrlC(int ) {
    gContinue = false;
}

int main () {
    gContinue = true;

    signal(SIGINT, handleCtrlC);

   ... allocate memory ...

    sigset_t sigmask;
    sigemptyset (&sigmask);  

    while (gContinue) {

        /*...*/
        ready = pselect(nfds, &readfds, &writefds, &exceptfds,
            timeout, &sigmask);
        /*...*/
    }

    ... free memory ...

    return 0;
}

Edit: Added pselect to the loop.

Adam Hunt
  • 129
  • 1
  • 6
  • I think `gContinue` has to be `volatile`. I don't know who told you global variables are "bad", but you should stop listening to them. This is exactly the kind of problem global variables are good for. – Dietrich Epp Jul 15 '12 at 00:21
  • Thanks. I'll add volatile and remove the comment. – Adam Hunt Jul 15 '12 at 00:27
  • BTW, I don't have the ability to comment on the other answer, but I think you don't need sigprocmask when using pselect. This is new information to me, but I think sigprocmask is used with select like this. sigprocmask(SIG_SETMASK, &sigmask, &origmask); ready = select(nfds, &readfds, &writefds, &exceptfds, timeout); sigprocmask(SIG_SETMASK, &origmask, NULL); – Adam Hunt Jul 15 '12 at 00:30
  • The idea behind `pselect` is that you can block signals, check the flag, then unblock them during the `pselect` call. If you block signals during the `pselect` call, as in your current example, the call won't be interrupted. But you *want* to interrupt the call. So you'll typically pass `pselect` a relatively empty signal mask. – Dietrich Epp Jul 15 '12 at 00:47
0

Adam Hunt's suggestion will work, provided that you add a check for the flag just before your call to select. It must be immediately before because your normal EINTR handling would likely skip over the check otherwise, so the exiting would be delayed until your next select returned a real event.

while (gContinue) {
    /*set up some stuff for next select call*/
    do {
        if (gContinue == 0) break;
        nfds = select(...);
    } while (nfds == -1 && errno == EINTR);
    /*handle select return*/
}

Edit: Dietrich Epp points out there is a race condition between the flag check and the select call that can only be closed by a call to pselect. pselect is very similar to select, with the primary difference being the last parameter is used as a mask to determine which signals to block. So the code sample below closes the race between the flag check and the pselect call:

sigset_t emptyset, blockset, origset;
sigemptyset(&emptyset);
sigemptyset(&blockset);
sigaddset(&blockset, SIGINT);

while (gContinue) {
    /*...*/
    sigprocmask(SIG_BLOCK, &blockset, &origset);
    do {
        if (gContinue == 0) break;
        nfds = pselect(..., &emptyset);
    } while (nfds == -1 && errno == EINTR);
    sigprocmask(SIG_SETMASK, &origset, NULL);
    /*...*/
};

Another approach is to register all your allocated elements to a global data structure, so that they can be freed by an atexit handler that you install. If you end up in code that will deallocate it, unregister it from the global data structure first;

m = malloc(sz);
register_allocation(m);
/*...*/
unregister_allocation(m);
free(m);

And using atexit:

void cleanup_allocations () {
    /*...*/
}

atexit(cleanup_allocations);
jxh
  • 69,070
  • 8
  • 110
  • 193
  • Note that this kind of situation is exactly why we have `pselect`. You can't avoid a race between checking the flag and calling `select`, so you **must** use `pselect` unless you want to lose some of your SIGINT. – Dietrich Epp Jul 14 '12 at 23:55
  • The `atexit` thing is just silly though, since all you're doing is quashing Valgrind messages without figuring out if they're real leaks or not. You might as well just disable Valgrind's leak checking, since that's what you're doing. – Dietrich Epp Jul 14 '12 at 23:57
  • @DietrichEpp: I'll edit my answer for `pselect`, thanks for the info. The `atexit` usage just depends on the goals of the OP. It could be used as an independent memory manager with its own leak detection that is always on and queryable, even in deployed code. – jxh Jul 15 '12 at 00:04