Changing the value of an integer using different processes in C, what's going wrong?

Question

I have an integer set to 0, I want to increment it by 10000 using the first process and by 5000 using each of 2 other processes I create.

Current code:

#include<stdio.h>
#include<stdlib.h>
#include<sys/mman.h>
#include<unistd.h>

int main() {
  int* shared_int = (int*) mmap(NULL, sizeof(int), PROT_READ | PROT_WRITE,
      MAP_SHARED | MAP_ANONYMOUS, -1, 0);
  *shared_int = 0;
  if (shared_int == MAP_FAILED) {
    perror("mmap failure");
    exit(EXIT_FAILURE);
  }

  pid_t pid = fork();

  if (pid > 0) {
    *shared_int = *shared_int + 10000;
    printf("Value after P1 incremention: ");
    printf("%d", *shared_int);
    exit(0);
  }

  pid_t pid2 = fork();

  if (pid == 0) {
    *shared_int = *shared_int + 5000;
    printf("Value after P2 incremention: ");
    printf("%d", *shared_int);
    exit(0);
  }

  if (pid2 == 0) {
    *shared_int = *shared_int + 5000;
    printf("Value after P3 incremention: ");
    printf("%d", *shared_int);
  }
  return 1;
}

Intended output:

Value after P1 incremention: 10000 
Value after P2 incremention: 15000
Value after P3 incremention: 20000

Current Output:

Value after P1 incremention: 10000 
Value after P2 incremention: 15000
Value after P2 incremention: 15000

I tried entering a random printf statement at the start of my third if and it didn't execute so it seems like the third process is terminated before that and is entering the second if when it shouldn't.

Answer 1

Diagnosis

There are a variety of problems, most of them diagnosed in the comments.

One undiagnosed problem is that you set *shared_int = 0; before checking that the memory mapping succeeded. Don't!

I changed this after I tried putting:

pid_t pid = fork();
pid_t pid2;
if (pid > 0)
    pid2 = fork();
int* shared_int = (int*) mmap(NULL, sizeof(int), PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0);

and I got an even worse output:

Value after P1 incremention: 10000
Value after P2 incremention: 5000
Value after P1 incremention: 10000 

Doing the mapping after the forks means that each process has its own, wholly unshared segment of memory. The code in the question is correct in setting up the memory before doing any forking.

You should include a newline at the end of the format string for each printf() pair, and each pair should be a single printf() — there is no advantage to separating the main 'comment' string from the formatting for the number. However, that's mechanics. The claimed output doesn't match the output from the code because of the discrepancy in the newlines.

[Partially relevant comment] Consider inserting wait(NULL); (ugh — that's the lazy way of doing it, but it works) between if (pid == 0) { … } and if (pid2 == 0) { … }. That forces P3 to wait for P2 to complete, I think. And should, therefore, avoid both processes reading the memory at the same time and setting it to the same value. Also, make one of the processes increment by 2500 or 7500 (or any value other than 10000 or 5000); it might make things easier to understand. Perhaps capture and print the 'before' value as well as the expected and actual after values. Multicore machines really do run things in parallel.

Also, the P1 process should probably wait for its child to complete before exiting.

Output didn't change, as mentioned in my question it seems like if (pid2 == 0) { … } isn't executing in the first place.

Well, yes — now you mention it again — unless the fork() failed so pid is -1, when you reach the point of pid2 = fork();, pid is 0 (and the process is the child process of the one that was launched originally). So after pid2 = fork();, pid is still 0 in both the child and the grandchild processes, so both of them execute the if (pid == 0) { … } block of code.

The first step in debugging code if you're not going to run a debugger is to print everything that might be relevant — especially PID values. Use getpid() to identify which process is printing the PID variables

printf("PID %d: pid = %d, pid2 = %d\n", (int)getpid(), (int)pid, (int)pid2);

The (int) casts are me covering my butt; there's no defined format specifier for pid_t (sadly; POSIX should extend <inttypes.h> to provide format specifiers for its types), and if pid_t is a typedef for long long (not impossible, though somewhat improbable), using %d would give problems. The casts ensure correct matching of format specifier and value, at the risk of chopping off critical information if the type actually is long long and a PID is more than 2³¹-1.

An alternative to the casts would be:

static_assert(sizeof(pid_t) == sizeof(int), "pid_t is not equivalent to int; fix the printf formats");

and then blithely ignore the problem.

Prescription

This code puts many of the ideas outlined in the diagnosis into effect:

#include <assert.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/mman.h>
#include <sys/wait.h>
#include <unistd.h>

static_assert(sizeof(pid_t) == sizeof(int), "pid_t is not equivalent to int; fix printf formats");

static void wait_for_kids(void);

int main(void)
{
    int *shared_int = (int *) mmap(NULL, sizeof(int), PROT_READ | PROT_WRITE,
                                   MAP_SHARED | MAP_ANONYMOUS, -1, 0);
    if (shared_int == MAP_FAILED)
    {
        perror("mmap failure");
        exit(EXIT_FAILURE);
    }
    *shared_int = 0;

    pid_t pid = fork();
    if (pid < 0)
    {
        perror("fork failure");
        exit(EXIT_FAILURE);
    }

    if (pid > 0)
    {
        *shared_int = *shared_int + 10000;
        printf("Value after P1 increment: %d\n", *shared_int);
        wait_for_kids();
        exit(0);
    }

    assert(pid == 0);

    pid_t pid2 = fork();
    if (pid2 < 0)
    {
        perror("fork failure");
        exit(EXIT_FAILURE);
    }

    if (pid2 > 0)
    {
        *shared_int = *shared_int + 5000;
        printf("Value after P2 increment: %d\n", *shared_int);
        wait_for_kids();
        exit(2);
    }

    assert(pid == 0 && pid2 == 0);
    {
        *shared_int = *shared_int + 2500;
        printf("Value after P3 increment: %d\n", *shared_int);
        wait_for_kids();    /* But no kids to wait for! */
        exit(3);
    }
    /*NOTREACHED*/
    return 32;
}

static void wait_for_kids(void)
{
    int status;
    int corpse;
    while ((corpse = wait(&status)) > 0)
        printf("%d: PID %d exited with status 0x%.4X\n", getpid(), corpse, status);
}

Sample output (tested on a MacBook Pro running macOS Mojave 10.14.6 with XCode 11.3.1 and using a home-compiled GCC 9.3.0):

Value after P1 increment: 10000
Value after P2 increment: 15000
Value after P3 increment: 17500
7439: PID 7440 exited with status 0x0300
7438: PID 7439 exited with status 0x0200

This is what you wanted, I believe (give or take the 'exited' messages).

Using functions

Here's a variant of the program above that encapsulates the main printing etc into a function. It uses some error reporting code that is available in my SOQ (Stack Overflow Questions) repository on GitHub as files stderr.c and stderr.h in the src/libsoq sub-directory.

#include "stderr.h"
#include <assert.h>
#include <stdio.h>
#include <stdlib.h>
#include <stdnoreturn.h>
#include <sys/mman.h>
#include <sys/wait.h>
#include <unistd.h>

static_assert(sizeof(pid_t) == sizeof(int), "pid_t is not equivalent to int; fix printf formats");

static void wait_for_kids(void);
static noreturn void increment_memory(int *mem, int pid, int inc, int status);

int main(int argc, char **argv)
{
    err_setarg0(argv[0]);
    if (argc != 1)
        err_usage("");
    err_setlogopts(ERR_PID);

    int *shared_int = (int *) mmap(NULL, sizeof(int), PROT_READ | PROT_WRITE,
                                   MAP_SHARED | MAP_ANONYMOUS, -1, 0);
    if (shared_int == MAP_FAILED)
        err_syserr("mmap failed: ");
    *shared_int = 0;

    pid_t pid = fork();
    if (pid < 0)
        err_syserr("fork failed: ");

    if (pid > 0)
        increment_memory(shared_int, 1, 10000, 0);

    assert(pid == 0);

    pid_t pid2 = fork();
    if (pid2 < 0)
        err_syserr("fork failed: ");

    if (pid2 > 0)
        increment_memory(shared_int, 2, 5000, 2);

    assert(pid == 0 && pid2 == 0);
    increment_memory(shared_int, 3, 2500, 2);

    /*NOTREACHED*/
    return 32;
}

static void wait_for_kids(void)
{
    int status;
    int corpse;
    while ((corpse = wait(&status)) > 0)
        printf("%d: PID %d exited with status 0x%.4X\n", getpid(), corpse, status);
}

static void increment_memory(int *mem, int num, int inc, int status)
{
    int old = *mem;
    *mem += inc;
    printf("%d: [%5d, %5d, %5d] Value after P%d increment: %d\n", getpid(), old, inc, old + inc, num, *mem);
    wait_for_kids();
    exit(status);
}

This has more detailed information in the printing. Example output:

13403: [    0, 10000, 10000] Value after P1 increment: 10000
13404: [10000,  5000, 15000] Value after P2 increment: 15000
13405: [15000,  2500, 17500] Value after P3 increment: 17500
13404: PID 13405 exited with status 0x0200
13403: PID 13404 exited with status 0x0200

I also created a variant of this with the call to wait_for_kids() before the printf() statement instead of after it. It generates, for example:

13408: [15000,  2500, 17500] Value after P3 increment: 17500
13407: PID 13408 exited with status 0x0200
13407: [10000,  5000, 15000] Value after P2 increment: 17500
13406: PID 13407 exited with status 0x0200
13406: [    0, 10000, 10000] Value after P1 increment: 17500

You'd get still different results if you placed the wait_for_kids() call before modifying the shared memory.

13411: [    0,  2500,  2500] Value after P3 increment: 2500
13410: PID 13411 exited with status 0x0200
13410: [ 2500,  5000,  7500] Value after P2 increment: 7500
13409: PID 13410 exited with status 0x0200
13409: [ 7500, 10000, 17500] Value after P1 increment: 17500

Beware of piping the output from such programs — it changes the output from line buffered to fully buffered.

Answer 2

the following proposed code:

1. cleanly compiles
2. properly checks for errors
3. performs the desired functionality
4. uses '\n' in the calls to printf() format strings so data is immediately output to the terminal
5. parent process properly waits for child process to exit
6. corrects the coding/logic problems as exposed in the comments to the question
7. contains appropriate horizontal/vertical spacing for readability

Note: this kind of statement:

*shared_int = *shared_int + 10000;

can be shortened to:

*shared_int += 10000;

And now, the proposed code:

#include <stdio.h>
#include <stdlib.h>
#include <sys/mman.h>
#include <unistd.h>

#include <sys/types.h>
#include <sys/wait.h>

int main( void ) 
{
    int* shared_int = (int*) mmap( NULL, sizeof(int), 
        PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0 );

    if ( shared_int == MAP_FAILED )
    {
        perror( "mmap failure" );
        exit( EXIT_FAILURE );
    }

    *shared_int = 0;

    pid_t pid = fork();

    switch( pid )
    {
        case -1:
            perror( "fork1 failed" );
            exit( EXIT_FAILURE );
            break;

        case 0:
            //child1
            *shared_int = *shared_int + 5000;
            printf( "Value after C1 incremention: " );
            printf( "%d\n", *shared_int );
            exit( EXIT_SUCCESS );
            break;

        default:
            // parent
            waitpid( pid, NULL, 0 );
            *shared_int = *shared_int + 10000;
            printf( "Value after P1 incremention: " );
            printf( "%d\n", *shared_int );
            break;
    }  // end switch

    // in parent process

    pid_t pid2 = fork();

    switch( pid2 )
    {
        case -1:
            perror( "fork2 failed" );
            exit( EXIT_FAILURE );
            break;

        case 0:
            // child2
            *shared_int = *shared_int + 5000;
            printf( "Value after C2 incremention: " );
            printf( "%d\n", *shared_int );
            break;

        default:
            // parent
            waitpid( pid2, NULL, 0 );
            break;
    }  //end switch

    return 0;
}

The output from the above code (which is consistent between runs):

Value after C1 incremention: 5000
Value after P1 incremention: 15000
Value after C2 incremention: 20000

Note: the GNU C library describes a pid_t as:

The pid_t data type is a signed integer type which is capable of representing a process ID. In the GNU C Library, this is an int.