IPC using pipes

Question

I have simulated the conversation between a caller and receiver using fork and pipe. The parent process is the receiver and the child process is the caller. Each message is terminated by a newline.
The program seems to be running correctly. Should I take care of any other condition?

#include <stdio.h>
#include <string.h>
#include <stdbool.h>
#include <sys/types.h>
#include <unistd.h>

#define BUF_LEN 512
#define READ_END 0
#define WRITE_END 1

int main()
{
    int fd[2];
    if (pipe(fd) == -1) {
        fprintf(stderr, "Pipe failed");
        return 1; 
    }
    pid_t pid = fork();

    if (pid < 0) {
        fprintf(stderr, "Fork failed");
        return 1;
    } 

    // the parent process is the receiver
    if (pid > 0) {
        close(fd[WRITE_END]);
        char buffer[BUF_LEN + 1] = "";
        bool end_call = false;
        do {
            // strcpy(buffer, "");
            if (read(fd[READ_END], buffer, sizeof buffer) != 0) {
                // char *start = buffer;
                char msg[BUF_LEN + 1] = "";
                char *end = buffer;
                int i = strlen(msg);
                while (*end) {
                    msg[i++] = *(end++);
                    msg[i] = '\0';
                    if (msg[strlen(msg) - 1] == '\n') {
                        printf("Receiver: Received %s", msg);
                        if (!strcmp(msg, "Bye!\n")) {
                            end_call = true;
                        }
                        strcpy(msg, "");
                        i = strlen(msg);
                    }
                }
                // if (strcmp(buffer, "")) {
                //  printf("Received %s", buffer);
                // }
            }
        } while (!end_call);
        close(fd[READ_END]);
    } else {
        close(fd[READ_END]);
        // const char *msg = "Hello";
        char buffer[BUF_LEN + 1] = "";
        bool end_call = false;
        printf("Caller: Enter messages to be sent to the receiver."
                "(\"Bye!\" to end call)\n");
        do {
            // printf("Caller: ");
            fgets(buffer, sizeof buffer, stdin);
            if (!strcmp(buffer, "Bye!\n")) {
                end_call = true;
            }
            // printf("Sent %s\n", buffer);
            write(fd[WRITE_END], buffer, strlen(buffer) + 1);
        } while (!end_call);
        close(fd[WRITE_END]);
    }
    return 0;
}

Answer 1

So many calls to strlen()

Let's count the calls to strlen() in this block of code:

        if (read(fd[READ_END], buffer, sizeof buffer) != 0) {
            // char *start = buffer;
            char msg[BUF_LEN + 1] = "";
            char *end = buffer;
            int i = strlen(msg);
            while (*end) {
                msg[i++] = *(end++);
                msg[i] = '\0';
                if (msg[strlen(msg) - 1] == '\n') {
                    printf("Receiver: Received %s", msg);
                    if (!strcmp(msg, "Bye!\n")) {
                        end_call = true;
                    }
                    strcpy(msg, "");
                    i = strlen(msg);
                }
            }
            // if (strcmp(buffer, "")) {
            //  printf("Received %s", buffer);
            // }
        }

1. The first call is at the beginning, where msg is still empty. Therefore, that line can be replaced with i = 0.
2. The second call is checking the last character for a newline. But you just added that character at msg[i-1], so you don't need to call strlen().
3. The third call happens right after you set msg to "". So you know the length will be zero here. You also don't need to call strcpy() just to clear out msg.

The revised code, with zero calls to strlen() and strcpy(), is this:

        if (read(fd[READ_END], buffer, sizeof buffer) != 0) {
            char msg[BUF_LEN + 1] = "";
            char *end = buffer;
            int i = 0;
            while (*end) {
                msg[i++] = *(end++);
                msg[i] = '\0';
                if (msg[i - 1] == '\n') {
                    printf("Receiver: Received %s", msg);
                    if (!strcmp(msg, "Bye!\n")) {
                        end_call = true;
                    }
                    msg[0] = '\0';
                    i = 0;
                }
            }
        }

Potential buffer overflow

In your actual program, since you control both the client and the server, there is no actual buffer overflow. But if you look at only the server part, there is a potential problem:

    char buffer[BUF_LEN + 1] = "";
    if (read(fd[READ_END], buffer, sizeof buffer) != 0) {

Here, you call read with sizeof buffer, meaning it can fill the entire buffer without having a terminating null character.

Then you iterate through the buffer with:

        char *end = buffer;
        while (*end) {

which means if there was no null terminating character, you will read off the end of the buffer.

It looks like you sized the buffer to be BUF_LEN + 1 for a reason (to leave room for a definite null character), so you should just call read() with one less length:

    if (read(fd[READ_END], buffer, sizeof(buffer)-1) != 0) {

Answer 2

• Any time you feel like putting a comment like

      // the parent process is a receiver
  

  delegate the commented functionality to a function:

      if (pid > 0) {
          receiver(...);
      }
  

  and remove a comment. The code becomes self-documented.

• The receiver logic is a bit weird. On error read returns -1, and once the error happens it will most likely keep returning -1, making the receiver stuck in the infinite loop. I recommend to

      int rc;
      while ((rc = read(....)) > 0) {
          do_receiver_logic(....);
      }
      if (rc == 0) {
          handle_peer_close();
      } else {
          handle_read_error();
      }
  

  You may want to pay special attention to transient errors, but I'd rather try to reconnect.

• The sender logic depends on a good behaviour of a message generator. If instead of saying "Bye" it closes the stream, the sender is doomed to repeatedly send the last string forever. At least, test that fgets returns not NULL.