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>

• Komgcn: just out of curiosity, is it not async-signal-safe to call shmdt()/shmctl() in the signal handler function?
• o11c: @Komgcn System calls are almost definitionally async-signal-safe in practice, being atomic from the program's perspective (barring syscalls that read/write to a memory buffer if someone else might be using it); the main gotcha is that certain functions despite being documented in section 2 are actually libc wrapper functions (fork is a particularly nasty case; setuid does voodoo to mostly work). Note that the modern SHM API is explicitly unsafe, whereas the legacy one is not mentioned at all.