Protecting a shared memory segment with semaphore does not work

Question

I have a program that creates 1000 child processes. Each process must have access to the int variable, which is stored in the shared memory segment. To protect the int variable, I created a semaphore:

#define _XOPEN_SOURCE #include <stdio.h> #include <stdlib.h> #include <unistd.h> #include <fcntl.h> #include <sys/types.h> #include <sys/wait.h> #include <sys/sem.h> #include <sys/ipc.h> #include <sys/shm.h> #define KEY 1000 #define LOCK -1 #define UNLOCK 1 int *ptr; int pid; int shm_id; int sem_id; struct sembuf sema; int main() { if( ( sem_id = semget( KEY, 1, IPC_CREAT | 0666 ) ) < 0 ) { printf( "semid error\n" ); exit( EXIT_SUCCESS ); } sema.sem_num = 1; sema.sem_op = 0; sema.sem_flg = SEM_UNDO; if( ( shm_id = shmget( KEY, 1, IPC_CREAT | 0666 ) ) < 0 ) { printf( "ERROR\n" ); exit( EXIT_SUCCESS ); } ptr = shmat( shm_id, NULL, 0 ); *ptr = 0; for( int i = 0; i < 10; ++i ) { pid = fork(); if( pid == 0 ) { // critical part sema.sem_op = LOCK; if( semop( sem_id, &sema, 1 ) < 0 ) { printf( "ERROR\n" ); } ++( *ptr ); sema.sem_op = UNLOCK; if( semop( sem_id, &sema, 1 ) < 0 ) { printf( "ERROR\n" ); } // end of the critical part exit( EXIT_SUCCESS ); } } int return_stat; enum { debug = 1 }; int corpse; while ( ( corpse = waitpid( ( pid_t )-1, &return_stat, 0 ) ) > 0 ) if ( debug ) printf( "PID %d died 0x%.4X\n", corpse, return_stat ); //while( waitpid( pid, &return_stat, 0 ) == 0 ); printf( "value %d\n", *ptr ); shmdt( NULL ); semctl( sem_id, 1, IPC_RMID, 0 ); } 

Here is an example output:

 PID 7288 died 0x0000 PID 7289 died 0x0000 PID 7290 died 0x0000 PID 7291 died 0x0000 PID 7292 died 0x0000 PID 7293 died 0x0000 PID 7294 died 0x0000 PID 7295 died 0x0000 PID 7296 died 0x0000 PID 7297 died 0x0000 value 9 PID 7276 died 0x0000 PID 7277 died 0x0000 PID 7278 died 0x0000 PID 7279 died 0x0000 PID 7280 died 0x0000 PID 7281 died 0x0000 PID 7282 died 0x0000 PID 7283 died 0x0000 PID 7284 died 0x0000 PID 7285 died 0x0000 value 10 

The output should be 1000 each time, but the output changes. I do not know why this piece of code is not working correctly. Can someone help me with my problem? Thanks you

Answer 1

Incorrect process cleaning cycle:

 while( waitpid( pid, &return_stat, 0 ) == 0 ); 

Since waitpid() returns the PID it is reporting to, this is not the cycle you want - it only waits for one PID to die. This may be what you need:

 enum { debug = 1 }; int corpse; while ((corpse = waitpid((pid_t)-1. &return_stat, 0)) > 0) { if (debug) printf("PID %d died 0x%.4X\n", corpse, return_stat); } 

You can set debug = 0 when you are sure that it is working correctly.

---

View Output Example

Another problem is the child code:

  if( pid == 0 ) { // critical part sema.sem_op = LOCK; if( semop( sem_id, &sema, 1 ) < 0 ) ++( *ptr ); sema.sem_op = UNLOCK; if( semop( sem_id, &sema, 1 ) < 0 ) // end of the critical part exit( EXIT_SUCCESS ); } 

You increase the pointer only if the first semop() fails; you exit (successfully) only if the second semop() .

You must exit unconditionally. You should only increment if the first semop() succeeds, and you should only do the second semop() if the first is executed. You will probably need the error message code after the if .

---

Another version

The residual problem that I see is that you have inverted the LOCK and UNLOCK values.

 #define _XOPEN_SOURCE #include <errno.h> #include <fcntl.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <sys/ipc.h> #include <sys/sem.h> #include <sys/shm.h> #include <sys/wait.h> #include <unistd.h> #define KEY 1000 #define LOCK +1 #define UNLOCK -1 static const char *arg0 = 0; static void err_setarg0(char *argv0) { arg0 = argv0; } static void err_syserr(const char *msg) { int errnum = errno; fprintf(stderr, "%s: %s", arg0, msg); if (errnum != 0) fprintf(stderr, " (%d: %s)", errnum, strerror(errnum)); fputc('\n', stderr); exit(EXIT_FAILURE); } int main(int argc, char **argv) { int *ptr; int pid; int shm_id; int sem_id; struct sembuf sema; err_setarg0(argv[argc-argc]); if ((sem_id = semget(KEY, 1, IPC_CREAT | 0666)) < 0) err_syserr("semget()"); sema.sem_num = 0; sema.sem_op = 0; sema.sem_flg = SEM_UNDO; if ((shm_id = shmget(KEY, 1, IPC_CREAT | 0666)) < 0) err_syserr("shmget()"); ptr = shmat(shm_id, NULL, 0); if (ptr == (int *)-1) err_syserr("shmat()"); *ptr = 0; printf("Looping\n"); for (int i = 0; i < 10; ++i) { pid = fork(); if (pid < 0) err_syserr("fork()"); else if (pid == 0) { // critical part sema.sem_op = LOCK; if (semop(sem_id, &sema, 1) < 0) err_syserr("semop() lock"); ++(*ptr); sema.sem_op = UNLOCK; if (semop(sem_id, &sema, 1) < 0) err_syserr("semop() unlock"); // end of the critical part exit(EXIT_SUCCESS); } } printf("Looped\n"); int return_stat; enum { debug = 1 }; int corpse; while ((corpse = waitpid((pid_t)-1, &return_stat, 0)) > 0) { if (debug) printf("PID %d died 0x%.4X\n", corpse, return_stat); } printf("value %d\n", *ptr); if (shmdt(ptr) == -1) err_syserr("shmdt()"); if (semctl(sem_id, 1, IPC_RMID, 0) == -1) err_syserr("semctl()"); if (shmctl(shm_id, IPC_RMID, 0) == -1) err_syserr("shmctl()"); return 0; } 

Execution Example:

 $ ./semop Looping Looped PID 17976 died 0x0000 PID 17977 died 0x0000 PID 17978 died 0x0000 PID 17979 died 0x0000 PID 17980 died 0x0000 PID 17981 died 0x0000 PID 17982 died 0x0000 PID 17983 died 0x0000 PID 17984 died 0x0000 PID 17985 died 0x0000 value 10 $ 

Note the use of err_syserr() to simplify error reporting. Along with err_setarg0() it is part of a large package of error reporting functions that I use regularly. In fact, my normal version is a printf like function with a format string and a list of variable arguments, but this simple version is suitable for this program and simpler.

Answer 2

I tried my code (after solving the connection of all the children), and all the children are waiting on the -1 semi-trailer. This is due to the fact that the semaphore is created with the initial value set to 0, but in order for one of them to start, it must be equal to 1.

From the semget Linux help system:

  The values of the semaphores in a newly created set are indeterminate. (POSIX.1-2001 is explicit on this point.) Although Linux, like many other implementations, initializes the semaphore values to 0, a portable application cannot rely on this: it should explicitly initialize the semaphores to the desired values. 

For initialization you can use:

 if(semctl(sem_id, 0, SETVAL, 1) == -1) { perror("semctl"); exit(0); } 

This can also be achieved using semop +1 if the initial value is 0.

Please note that you can avoid interacting with other programs or the previous launch by using IPC_PRIVATE as the sem / shm key.