Freeing shared memory with a signal handler

Question

I have a homework question that asks me to get 2 integers from the user, put them in shared memory, fork a child, have the child add them and put the result in shared memory, and then the child will end and the parent will print out the sum. The program must also loop until the user does CTRL+C, at which point it should catch the signal and the parent should detach and remove shared memory, SIGKILL the child, and then terminate itself.

I've already submitted this, and it got full credit, but I can't help but feel there is a better way. Mainly, my concern is with the way I free the memory at the bottom of main() and, how I handle the signal. I really wanted to free the memory in the signal handler, but couldn't figure out how to do that without making all the shared variables globals. Those if (runFlag) statements don't seem particularly elegant to me either.

#define _POSIX_SOURCE

#include <stdio.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <string.h>
#include <sys/mman.h>
#include <fcntl.h>
#include <unistd.h>
#include <stdlib.h>
#include <signal.h>

int runFlag = 1; //this flag will be set to 0 by a SIGINT

/* This function creates a shared memory area in the parent's address space of size size_of_shared_memory, and returns a void* pointer to it */ 
void *create_shared_memory(int size_of_shared_memory){
  int fd;
  void *area;

  fd = open("/dev/zero", O_RDWR); 
  area = mmap(0, size_of_shared_memory, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
  close(fd);
  return area;
}


void intHandler()
{//toggle runFlag so main loop exits
    runFlag = 0;
    //shmdt(); //detaches shared memory
    //shmctl(shm_id, IPC_RMID, NULL); //removes shared memory

    return;
}

int main(int argc, char **argv)
{
    int *num1 = (int *)create_shared_memory(sizeof(int));
    int *num2 = (int *)create_shared_memory(sizeof(int));
    int *sum = (int *)create_shared_memory(sizeof(int));
    int *flag = (int *)create_shared_memory(sizeof(int));
    *flag = 1;
    int child_pid;

    while(runFlag)
    {//runFlag will be changed by a SIGINT
        //since runFlag could be changed anywhere we must test for it whereever the program will be sitting

        if (runFlag) printf("\nEnter the first number: ");
        if (runFlag) scanf("%d", num1);
        if (runFlag) printf("Enter the second number: ");
        if (runFlag) scanf("%d", num2);

        if (runFlag)
        {
            *flag = 1;
            child_pid = fork(); 
            signal(SIGINT, intHandler);

            if(child_pid < 0){
                printf("Fork error. \n");
                exit(-1);
            }
            if(child_pid == 0){ // Child code
                signal(SIGINT, SIG_IGN); //ignore ctrl+c
                *sum = *num1 + *num2;
                *flag = 0; //signal to parent that child is done
                exit(0);
            }

            /* Parent code */
            while (*flag); //wait for child to signal it is done


            /* Read the integers from shared memory */
            printf("The sum is: %d\n", *sum);
        }
    }
    //exit
    printf("\n");
    munmap(num1, sizeof(int)); //unmap memory
    munmap(num2, sizeof(int));
    munmap(sum, sizeof(int));
    munmap(flag, sizeof(int));
    if (child_pid > 0) kill(child_pid, SIGKILL); //kill child process
  exit(0); //kill parent
}

Answer 1

There are a lot of functions that aren't safe to call in a signal handler. For the few that are allowed, look at the Async-signal-safe functions section of the signal(7) man page.

Your current signal handler is almost okay, but any variable that might be changed from a signal handler needs to be declared volatile. Officially it must also be sig_atomic_t instead of int, though I'm not sure how important that is on real-world platforms.

You should be checking the return value of printf and scanf. They will return EOF (which is usually -1 but could theoretically be any negative number) with errno set to EINTR if the signal has been delivered during the underlying system call. That said, there is nothing to prevent the signal from being delivered before or after the syscall itself (before scanf is the nasty case, and doing the syscall yourself won't help unless the signal is blocked around the call).

If you want to handle signals in any sort of sane way, you need to sigblock them, at least some of the time. Either then unblock them during "safe" runs of code and check the flag periodically or else leave them blocked call sigwait (which will not invoke the handler).

---

My preference is to then use signalfd so that I can stuff it in a select or poll or epoll set along with any other read- or write- file descriptors.

---

There is no good reason for you to make 4 separate mmap calls. Most likely a single call for 4 * sizeof(int) would be enough, but even if you have a pressing need for them to be on separate pages, you should still mmap 4 pages in a single call.

You don't need to open "/dev/zero" to get anonymous memory, just pass -1 as the FD and add the MAP_ANONYMOUS flag.

---

Your child should call _exit, not exit.

---

PIDs should be stored in a variable of type pid_t.

---

Any memory that you read/write from multiple processes (or threads) must be protected either by some sort of mutex or by atomic operations with an appropriate memory order. It would not be enough to volatile.

So the following line may be optimized to be an infinite loop:

while (*flag);

Edit: It does indeed happen. Looking at the relevant assembly (at -O3):

# *flag = 1;
  400840:       mov    DWORD PTR [r12],0x1
# child_pid = fork();
  400848:       call   400720 <fork@plt>
  40084d:       mov    esi,0x4009c0
  400852:       mov    r14d,eax
  400855:       mov    edi,0x2
# signal(SIGINT, intHandler); - oh, this should be before the fork!
  40085a:       call   4006c0 <__sysv_signal@plt>
# 3-way branch for child_pid
  40085f:       test   r14d,r14d
  400862:       js     400885 <main+0x155>
  400864:       je     400897 <main+0x167>
# child_pid > 0
  400866:       mov    edx,DWORD PTR [r12]
# if (*flag) - condition only tested once
  40086a:       test   edx,edx
  40086c:       je     400870 <main+0x140>
# infinite loop
  40086e:       jmp    40086e <main+0x13e>
# else
  400870:       mov    esi,DWORD PTR [r13+0x0]
  400874:       mov    edi,0x400af2
  400879:       xor    eax,eax
# printf("The sum is: %d\n", *sum);
  40087b:       call   400680 <printf@plt>
  400880:       jmp    400772 <main+0x42>
# child_pid < 0
  400885:       mov    edi,0x400ae5
# printf("Fork error. \n");
  40088a:       call   400660 <puts@plt>
  40088f:       or     edi,0xffffffff
# exit(-1); - should use 1 or EXIT_FAILURE actually.
  400892:       call   400710 <exit@plt>
# child_pid == 0
  400897:       mov    edi,0x2
  40089c:       mov    esi,0x1
# signal(SIGINT, SIG_IGN); //ignore ctrl+c
  4008a1:       call   4006c0 <__sysv_signal@plt>
  4008a6:       mov    eax,DWORD PTR [rbx]
  4008a8:       add    eax,DWORD PTR [rbp+0x0]
  4008ab:       xor    edi,edi
# *sum = *num1 + *num2;
  4008ad:       mov    DWORD PTR [r13+0x0],eax
# *flag = 0;
  4008b1:       mov    DWORD PTR [r12],0x0
# exit(0);
  4008b9:       call   400710 <exit@plt>