Synchronization with child signal and parent matches received signals with the file

Question

EDIT: Thanks for the suggestions given so far. I changed the program and now the parent handles some of the signals, but it looks like it doesn't handle all of them. New code and results are posted below.

EDIT2: I changed the random number generation as proposed. Now the parent catches only two signals but it always catches the right bits (two last bits).

"Unfortunately, I am not experienced in C POSIX and I have to write a program that will take one argument (filename containing a binary number) and parse this file. Each bit denoted in the file means that one child should be created (each bit is dedicated to one child).
The value of the bit (0 or 1) decides which signals should be sent to the parent (0 - SIGUSR1, 1 - SIGUSR2).
The child process should choose a random interval (10-200ms) and send appropriate signal to the parent.

The parent should receive the signals and print the last 5 bits received every time a new signal arrives.

The final step is the matching process - the parent checks the received signals (bits assigned to SIGUSR1 or SIGUSR2) and if there's a match it prints SUCCESS. If there's no match (whenever a wrong bit is sent - compared to the file) the parent starts matching from the beginning."

The updated version:

#include <unistd.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/wait.h>
#include <sys/time.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <errno.h>
#include <string.h>
#include <time.h>
#include <signal.h>
volatile sig_atomic_t last_signal = 0;

char * str;
char * received;
int count = 0;

#define ERR(source) (fprintf(stderr,"%s:%d\n",__FILE__,__LINE__),\
                     perror(source),kill(0,SIGKILL),\
                                     exit(EXIT_FAILURE))

void sethandler( void (*f)(int), int sigNo)
{
        struct sigaction act;
        memset(&act, 0, sizeof(struct sigaction));
        act.sa_handler = f;
        if (-1==sigaction(sigNo, &act, NULL)) ERR("sigaction");
}


char *readFile(char *fileName)
{
    FILE *file = fopen(fileName, "r");
    char *code;
    size_t n = 0;
    int c;

    if (file == NULL) return NULL; //could not open file
    fseek(file, 0, SEEK_END);
    long f_size = ftell(file);
    fseek(file, 0, SEEK_SET);
    code = malloc(f_size);
    received = malloc(f_size);
    while ((c = fgetc(file)) != EOF) {
        code[n++] = (char)c;
    }

    code[n] = '\0';

    return code;
}

void append(char* s, char c)
{
        int len = strlen(s);
        s[len] = c;
        s[len+1] = '\0';
}

static void sig_handle(int signum)
{
  last_signal = signum;
}

void child_w(int number_of)
{
    if(str[number_of] == '0')
    {
      if (kill(getppid(), SIGUSR1)==0) printf("[SIGUSR1] sent \n");
      else
      {
        printf("ERROR kill. \n");
        exit(EXIT_FAILURE);
      }
    }

    if(str[number_of] == '1')
    {
      if (kill(getppid(), SIGUSR2) == 0) printf("[SIGUSR2] sent \n");
      else
      {
        printf("ERROR kill. \n");
        exit(EXIT_FAILURE);
      }
    }
}

void create_children(int n)
{
  pid_t s;
  int j = n;
  int time = rand() % 191 + 10;  // range 10 - 200
  struct timespec time_wait = { .tv_sec = 0, .tv_nsec = time * 1000000L };
       while(j-->0)
       {
         nanosleep(&time_wait, NULL);
         if((s=fork())<0) ERR("Fork ERROR");
         if(!s) {
                  printf("Child %d started ", j);
                  printf("with bit: %c \n", str[j]);
                  child_w(j);
                 exit(EXIT_SUCCESS);
         }
       }
}

void parent_w(sigset_t oldmask)
{
    int count = 0;
    int match = 0;
    while(1)
    {
      last_signal = 0;
      while(last_signal != SIGUSR1 && last_signal != SIGUSR2)
      {
        sigsuspend(&oldmask);
      }
      printf("\n");
      if(last_signal == SIGUSR1)
      {
        received[count] = '0';
        for(int i=0; i<sizeof(received); ++i)
        {
          printf("%c ", received[i]);
        }
        count++;
      }

      else if(last_signal == SIGUSR2)
      {
        received[count] = '1';
        for(int i=0; i<sizeof(received); ++i)
        {
          printf("%c ", received[i]);
        }
        count++;
      }
      printf("\n");
    }
}

int main(int argc, char ** argv)
{
  char filename[250];

  if(argc!=2)
  {
    printf("Provide one parameter - filename. \n");
    return EXIT_FAILURE;
  }
  strcpy(filename, argv[1]);

  str = readFile(filename);
  printf("FILE: ");
  for(int i=0; i<sizeof(str); ++i)
  {
    printf("%c ", str[i]);
  }
  printf("\n");

  for(int i=0; i<sizeof(received); ++i)
  {
    received[i] = '-';
  }

  sethandler(sig_handle, SIGUSR1);
  sethandler(sig_handle, SIGUSR2);

  sigset_t mask, oldmask;
    sigemptyset(&mask);
    sigaddset(&mask, SIGUSR1);
    sigaddset(&mask, SIGUSR2);
    sigprocmask(SIG_BLOCK, &mask, &oldmask);

  create_children(sizeof(str));

  parent_w(oldmask);

  sigprocmask(SIG_UNBLOCK, &mask, NULL);
  free(str);
  free(received);

  return EXIT_SUCCESS;
}

Now the output always looks like this:

FILE: 1 0 0 1 1 0 1 0 
Child 7 started with bit: 0 
[SIGUSR1] sent 
Child 6 started with bit: 1 
[SIGUSR2] sent 
Child 5 started with bit: 0 
[SIGUSR1] sent 
Child 4 started with bit: 1 
[SIGUSR2] sent 
Child 3 started with bit: 1 
[SIGUSR2] sent 
Child 2 started with bit: 0 
[SIGUSR1] sent 
Child 1 started with bit: 0 
[SIGUSR1] sent 

0 - - - - - - - 
Child 0 started with bit: 1 
[SIGUSR2] sent 

0 1 - - - - - - 

Any further suggestions will be appreciated :).

Answer 1

In addtition to the problems mentioned by others, your readFile() function invokes undefined behavior by overrunning the buffer you allocate for the file contents:

char *readFile(char *fileName)
{
    FILE *file = fopen(fileName, "r");
    char *code;
    size_t n = 0;
    int c;

    if (file == NULL) return NULL; //could not open file
    fseek(file, 0, SEEK_END);
    long f_size = ftell(file);
    fseek(file, 0, SEEK_SET);
    code = malloc(f_size);
    received = malloc(f_size);
    while ((c = fgetc(file)) != EOF) {
        code[n++] = (char)c;
    }

    code[n] = '\0';  // <- this is f_size + 1 bytes into the code array

    return code;
}

When you terminate the data with code[n] = '\0'; you write past the end of the buffer code points to, thus invoking undefined behavior.

And, off-topic...

Strictly speaking you can't use fseek()/ftell() to get the size of a file. In your case, you're opening the file in text mode with FILE *file = fopen(fileName, "r");, but in text mode ftell() does not return a byte offset. Per 7.21.9.4 The ftell function, paragraph 2 of the C11 standard:

The ftell function obtains the current value of the file position indicator for the stream pointed to by stream. For a binary stream, the value is the number of characters from the beginning of the file. For a text stream, its file position indicator contains unspecified information, usable by the fseek function for returning the file position indicator for the stream to its position at the time of the ftell call; the difference between two such return values is not necessarily a meaningful measure of the number of characters written or read.

On POSIX systems, you won't have a problem as POSIX defines ftell() to always return an accurate byte offset. But on Windows you will likely read fewer bytes than the file size would otherwise indicate as the \r\n character sequence actually in the file contents is read as a single \n character.

But on some systems you will truly get "unspecified information" and your code will fail completely.

And seeking to the end of a binary stream isn't portable either. In fact, it's explicitly undefined behavior:

Setting the file position indicator to end-of-file, as with fseek(file, 0, SEEK_END), has undefined behavior for a binary stream...

Again, not a problem on POSIX or Windows systems.

One real problem with fseek()/ftell(), though, is that the long value returned from ftell() doesn't have enough range on many systems to represent file sizes larger than 2 GB. long is 32 bits on 32-bit Linux systems, and its also only 32 bits on all Windows systems, both 32- and 64-bit.

Answer 2

As @KamilCuk observed, it is not necessary or appropriate to re-register signal handlers when they fire. That was standard at one time because of uncertainty about the implementation of the signal() function (which remains to this day): some implementations register handlers such that after they fire once, the signal disposition is reset. With sigaction(), however, once can specify whether they want that "one-shot" behavior or whether they instead want the signal handler to remain registered when it fires, with the latter being the default with that function.

sigaction() allows control of some other details on which signal() implementations vary. In practice, signal() has very few appropriate uses, and none that sigaction() cannot also cover. If you're programming for POSIX then its best to forget that signal() exists.

With all that said, however, I don't think your usage of signal() is the key problem here.

---

Another issue is that signal handlers are rather restricted in what they may safely do:

• they may access file-scope variables of type sig_atomic_t
• they may call async-signal-safe standard functions
• they may declare and access local variables
• they may call any of the program's other functions that comply with these restrictions

HOWEVER, signal handlers are typically called with their own, separate stacks, and these are often very small, so in practice, they cannot safely declare very much in the way of local variables, nor start a very deep call tree. Exactly what the limits are is unspecified, so signal handlers should generally do as little as possible.

In particular, neither printf() nor any of the other stdio functions are async-signal safe. Signal handlers produce undefined behavior if they call any of them. They may call write() if you wish, but there is probably a better alternative here. For example, the parent could pause() or sigsuspend() to await a signal, and then print whatever it needs to do outside the handler. The handler need only set a variable to indicate which signal was received. This will avoid the parent busy-waiting as it presently does, though you still have an issue with potential collisions.

This is more likely to be part of your problem, but I suspect that it's not the key issue, either.

---

I think the real problem is probably that signals are being lost. Ordinary signals do not queue, so if a signal is received while that signal is already pending for the process then it has no additional effect. The problem is structured to avoid that by asking for each child to delay a random amount of time before firing its signal, but

1. That's not truly safe, just less likely to manifest a collision, and
2. Your implementation does not actually delay.

Consider:

  int time = rand()%100 + 10;
  struct timespec time_wait = {time/1000, 0};
  nanosleep(&time_wait, NULL);

Variable time will be assigned a value between 10 and 109, so time / 1000 -- an integer division -- will always evaluate to 0.

Something like this would be more appropriate:

int time = rand() % 191 + 10;  // range 10 - 200
struct timespec time_wait = { .tv_sec = 0, .tv_nsec = time * 1000000L };
nanosleep(&time_wait, NULL);

Additionally, instead of seeding a separate (P)RNG in each child, I would seed one, once, in the parent, and generate the delays there, before each fork. Drawing random numbers from the same RNG produces a more uniform distribution.

Answer 3

First of all, good code.

Second:

• man pages are your friends. man signal
• signal() registers handler for the signal. So after signal(SIGUSR1, some_function) the function some_function will be executed after the signal is received.
• Removoe signal() calls from signal handlers (why would you re-register the same handler from inside the handler? It already is the handler for this signal.)
• Remove signal() call from the loop in parent. Just register functions once, that's all.
• Your sethandler function is the same as signal.

After some fixing:

#include <unistd.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/wait.h>
#include <sys/time.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <errno.h>
#include <string.h>
#include <time.h>
#include <signal.h>

char * str; // the array consisting of bits from the file
char * received; //array of the parent receiving the signals
int count = 0;
//Error macro
#define ERR(source) (fprintf(stderr,"%s:%d\n",__FILE__,__LINE__),\
                     perror(source),exit(EXIT_FAILURE))

//Reading file char by char and returning allocated char array
char *readFile(char *fileName)
{
    FILE *file = fopen(fileName, "r");
    char *code;
    size_t n = 0;
    int c;

    if (file == NULL) return NULL; //could not open file
    fseek(file, 0, SEEK_END);
    long f_size = ftell(file);
    fseek(file, 0, SEEK_SET);
    code = malloc(f_size);
    received = malloc(f_size);
    while ((c = fgetc(file)) != EOF) {
        code[n++] = (char)c;
    }

    code[n] = '\0';

    return code;
}
// Append the character to the received array
void append(char* s, char c)
{
        int len = strlen(s);
        s[len] = c;
        s[len+1] = '\0';
}
// SIGUSR1 handler. I tried to implement simple counter to check if the parent is receiving the signals, then proceed to printing last 5 bits received. Unfortunately this part seems to not work at all.
static void sig_handle1(int signum) {
  count++;
  printf("%s %d \n", __func__, count);

}
// Handler for SIGUSR2 - same as handler for SIGUSR1
static void sig_handle2(int signum) {
  count++;
  printf("%s %d \n", __func__, count);

}
// Child function - set the random interval, wait and then send the appropriate signal to the parent
void child_w(int number_of)
{
  srand(time(NULL)*getpid());
  int time = rand()%100 + 10;
  struct timespec time_wait = {time/1000, 0};
  nanosleep(&time_wait, NULL);

    if(str[number_of] == '0')
    {
      if (kill(getppid(), SIGUSR1)==0) printf("[SIGUSR1] sent \n");
      else
      {
        printf("ERROR kill. \n");
        exit(EXIT_FAILURE);
      }
    }

    if(str[number_of] == '1')
    {
      if (kill(getppid(), SIGUSR2) == 0) printf("[SIGUSR2] sent \n");
      else
      {
        printf("ERROR kill. \n");
        exit(EXIT_FAILURE);
      }
    }
}
// Function which will create children (number of children = number of bits in the file)
void create_children(int n)
{
  pid_t s;
  int j = n;
       while(j-->0)
       {
         if((s=fork())<0) ERR("Fork ERROR");
         if(!s) {
                  printf("Child %d started ", j);
                  printf("with bit: %c \n", str[j]);
                  child_w(j);
                 //if(j==1) kill(getppid(), SIGUSR2);
                 exit(EXIT_SUCCESS);
         }
       }
}
// Parent function to check the received signals
void parent_w()
{
        signal(SIGUSR1, sig_handle1);
        signal(SIGUSR2, sig_handle2);
  while(1)
  {
          pause();

  }
}

int main(int argc, char ** argv)
{
  char filename[250];

  if(argc!=2)
  {
    printf("Provide one parameter - filename. \n");
    return EXIT_FAILURE;
  }
  strcpy(filename, argv[1]);

  str = readFile(filename);
  printf("FILE: ");
  for(int i=0; i<sizeof(str); ++i)
  {
    printf("%c ", str[i]);
  }
  printf("\n");

  create_children(sizeof(str)-1);
  parent_w();


  free(str);
  free(received);

  return EXIT_SUCCESS;
}

Example exeuction:

FILE: 1 0 0 1 1 0 1

Child 0 started with bit: 1
Child 1 started with bit: 0
Child 2 started with bit: 0
Child 3 started with bit: 1
[SIGUSR2] sent
sig_handle2 1
[SIGUSR1] sent
sig_handle1 2
[SIGUSR1] sent
sig_handle1 3
[SIGUSR2] sent
sig_handle2 4
Child 4 started with bit: 1
Child 5 started with bit: 0
sig_handle2 5
[SIGUSR2] sent
Child 6 started with bit: 1
sig_handle1 6
[SIGUSR1] sent
sig_handle2 7
[SIGUSR2] sent
^C