I am getting incorrect output from my forks

Question

I'm attempting to create a program that creates 2 child processes, and 4 pipes (I know this isn't ideal, but the spec for this specific assignment requires it). While it correctly sorts two of the 5 command line argument integers, the rest are just spat out as what I believe are uninitialized integers, I.E. 7 is printed out as 33234951.

I'm pretty new to pipes, and it's been a little hard to wrap my head around, so I believe this issue has to do with this and not some arbitrary error in code.

I was able to successfully get this done using only 1 parent and child, but as soon as I tried to implement multiple, things got dicey.

I have a lot of unused includes just from messing around with things in attempt to solve the problem.

#include <bits/stdc++.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>
int main(int argc, char **argv) {

  printf("Starting\n");
  pid_t pid;
  pid_t pid2;
  int mypipe0[2];
  int mypipe1[2];
  int mypipe2[2];
  int mypipe3[2];

  pipe(mypipe0);
  pipe(mypipe1);
  pipe(mypipe2);
  pipe(mypipe3);

  /* Create the child process. */
  pid = fork();

  std::cout << "Fork " << pid << std::endl; 
  // Child: Sorts Array
  if (pid == 0) {

    printf("pid == (pid_t) 0 p2\n");

    /* This is the child process.
       Close other end first. */
    close(mypipe0[1]);
    char valuesArray[5];
    for (int a = 0; a < 5; a++)
      read(mypipe0[0], &valuesArray[a], sizeof(char));

    printf("finish reading mypipe0");
    std::sort(valuesArray, valuesArray + 5);

    printf("sorted");
    close(mypipe1[0]);
    close(mypipe2[0]);

    for (int a = 0; a < 5; a++) {
      write(mypipe1[1], &valuesArray[a], sizeof(char));
      write(mypipe2[1], &valuesArray[a], sizeof(char));
    }

    close(mypipe1[1]);
    close(mypipe2[1]);
    exit(0);
  } 

  else if (pid > 1) {
    std::cout << "pid == (pid_t) 1" << std::endl;
    /* This is the parent process.
        Close other end first. */
    close(mypipe0[0]); // Closes reading
    int valuesArray[5];
    valuesArray[0] = atoi(argv[1]);
    valuesArray[1] = atoi(argv[2]);
    valuesArray[2] = atoi(argv[3]);
    valuesArray[3] = atoi(argv[4]);
    valuesArray[4] = atoi(argv[5]);
    printf("Argv init");
    for (int a = 0; a < 5; ++a)
      write(mypipe0[1], &valuesArray[a], sizeof(char));

    printf("wrote to pipe 1");
    close(mypipe0[1]);

    wait(NULL);

    close(mypipe1[1]); // Closes writing
                       //            char outputArray[6];
    int sortedArray[5];
    for (int a = 0; a < 5; ++a)
      read(mypipe1[0], &sortedArray[a], sizeof(char));

    // Printing Array]
    for (int a = 1; a < 5; ++a)
      printf(", %d", sortedArray[a]);
    printf("]");

    //            wait(NULL);

    //            close(mypipe2[1]); // Closes writing
    //            int median;
    //            read(mypipe1[0], median, sizeof(charian));

    exit(0);
  }


  else {

    pid2 = fork();
    // Other child
    if (pid2 == 0) {

      printf("pid == (pid_t) 0\n");

      /* This is the child process.
         Close other end first. */
      close(mypipe0[1]);
      char valuesArray[5];
      for (int a = 0; a < 5; a++)
        read(mypipe0[0], &valuesArray[a], sizeof(char));

      printf("finish reading mypipe0");
      std::sort(valuesArray, valuesArray + 5);

      printf("sorted");
      close(mypipe1[0]);
      close(mypipe2[0]);

      for (int a = 0; a < 5; a++) {
        write(mypipe1[1], &valuesArray[a], sizeof(char));
        write(mypipe2[1], &valuesArray[a], sizeof(char));
      }

      close(mypipe1[1]);
      close(mypipe2[1]);
      exit(0);
    }
  }
}

I expect the output to be 2 4 5 6 7 from the given command line arguments of 4 2 5 6 7. Instead, I get [1, 28932, 5, 6, -14276913]

Be careful with `#include ` it does a lot more than you want it to do. It's a support file for precompiled headers, and you aren't intended to directly include it. Misusing it makes your code compile much more slowly rather than provide the speed-up you should get when used correctly. — user4581301, Nov 08 '19 at 18:13
Instead of `pipe1`...`pipe4` you should just use an array here. This code is at least twice as long as it needs to be due to rampant duplication that can't be avoided because of the choice of variables. This code also urgently needs some functions to split it up into smaller, more understandable parts, each of which has a specific purpose. — tadman, Nov 08 '19 at 18:15
@tadman Assignment spec requires us to deliberately declare them. This was provided in the initial code. — Brandon Clark, Nov 08 '19 at 18:18
@MutableSideEffect while writing this i realized this was probably part of the issue. It's definitely visiting the branch according to my print statements, but I don't know how else to go about getting the fork to the second child. — Brandon Clark, Nov 08 '19 at 18:19
@BrandonClark This assignment is really odd, and by odd I mean really out of line with how most C code is written. — tadman, Nov 08 '19 at 18:20
@tadman I've been told this a lot, and also been told that breaking this up into 2 child processes is not the norm. — Brandon Clark, Nov 08 '19 at 18:23
It'd still be worthwhile to carve this up into functions to better break down the big problem into smaller ones. — tadman, Nov 08 '19 at 18:25
`std::cout << "pid == (pid_t) 1" << std::endl;` what does this statement mean? pid is not `1` in `else if (pid > 1)` branch. Did you previously have the condition as `pid == 1`? — Empty Space, Nov 08 '19 at 18:25
@tadman I would, my largest issue I don't think I really understand what's going on with fork and pipes anymore. I thought I did, but I'm realizing thanks to the comment before that it really doesn't make sense why I'd just be using that top level else statement to get to the second child. Out of curiosity, would the better approach here be to fork within the parent process of the first fork in order to accomplish this? — Brandon Clark, Nov 08 '19 at 18:26
@tadman Yeah, I did. I forgot to update the print statement afterwards, but read on another stack post that the correct way to account for parents is with a value of > 1, not == 1 — Brandon Clark, Nov 08 '19 at 18:27
The better approach would be to create functions that have a singular, specific purpose in mind, and unit tests that verify these functions do their job correctly under their intended use cases. Flailing around with a huge tangle of code like this is never easy, even for seasoned developers. Divide and conquer. — tadman, Nov 08 '19 at 18:28
That explains why your print statements worked previously in else branch. — Empty Space, Nov 08 '19 at 18:29
@MutableSideEffect did I have the wrong takeaway? Is == 1 the correct way to account for parents? — Brandon Clark, Nov 08 '19 at 18:31
@BrandonClark No, you should never check for equality for the parent case. You said in a previous comment that `It's definitely visiting the branch according to my print statements`. This is strange. I think you had those print statements when you were checking if `pid` is 1 for the parent case, which is incorrect. — Empty Space, Nov 08 '19 at 18:35
**Recommended reading:** [Why should I not #include ?](https://stackoverflow.com/q/31816095/560648) — Lightness Races in Orbit, Nov 08 '19 at 18:42
@MutableSideEffect So just wondering, how do I ensure the second child process is visited correctly? I realize now that this isn't working how I thought it was. It definitely isn't being visited. — Brandon Clark, Nov 08 '19 at 18:44
@BrandonClark I thought you already solved the problem from your previous comment: `Out of curiosity, would the better approach here be to fork within the parent process of the first fork in order to accomplish this?` I wrote an answer explaining how you should structure your code instead. — Empty Space, Nov 08 '19 at 18:56
You need to ensure that you close everything you don't use. The first child inherits 4 pipes but only closes ends of 2 of them. That's going to burn you eventually, as you will get process blocked on reads since the write ends of the pipes aren't closed. Be diligent about closing *everything* you don't use. — William Pursell, Nov 08 '19 at 19:13

score 0 · Accepted Answer · answered Nov 08 '19 at 18:54

The else branch is reached when there is a failure in the first fork call.

Your current code structure is,

pid = fork();

if (pid == 0)
{
  runFirstChild();
}
else if (pid > 1)
{
  runParent();
}
else // This branch is not taken unless there is a failure
{
  pid2 = fork();
  if (pid2 == 0)
  {
    runSecondChild();
  }
}

You should instead structure it like so,

pid = fork();

if (pid == 0)
{
  runFirstChild();
}
else if (pid > 1)
{
  pid2 = fork(); // Fork the second child in the parent process
  if (pid2 == 0)
  {
    runSecondChild();
  }
  else if (pid2 > 1)
  {
    runParent();
  }
}

I am getting incorrect output from my forks

1 Answers1