Segfault on Server after Multithreading in C

Question

So I'm trying to code a multi-threading server. I've spent an enormous time on the internet figuring out the correct way to do this and the answer as always seems to be it depends. Whenever I execute my code, the client successfully connects, and executes but when the thread terminates and returns to the while loop the whole program segfaults.

I probably could use a good spanking on a few other things as well such as my usage of global variables. The entirety of code is below, sorry for the inconsistent space/tabbing.

#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <stdbool.h>
#include <signal.h>
#include <math.h>
#include <pthread.h>
#include <sys/stat.h>
#include <fcntl.h>

/* ---------------------------------------------------------------------
  This is a basic whiteboard server. You can query it, append to it and
  clear in it. It understands both encrypted and unencrypted data.
 --------------------------------------------------------------------- */

struct whiteboard {
    int line;
    char type;
    int bytes;
    char string[1024];
} *Server;

int serverSize, threadcount, id[5];
bool debug = true;
struct whiteboard *Server;

pthread_mutex_t mutex;
pthread_t thread[5];

/* -------------------------------------------
  function: sigint_handler
  Opens a file "whiteboard.all" in writemode 
  and writes all white board information in 
  command mode.
 ------------------------------------------- */
void sigint_handler(int sig) 
{
    if (debug) printf("\nInduced SIGINT.\n");

    FILE *fp;
    fp=fopen("whiteboard.all","w");

    int x=0;
    for (x;x<serverSize;x++) // Loop Responsible for iterating all the whiteboard entries.
    {
        if (debug) printf("@%d%c%d\n%s\n",Server[x].line,Server[x].type,Server[x].bytes,Server[x].string);
        fprintf(fp,"@%d%c%d\n%s\n",Server[x].line,Server[x].type,Server[x].bytes,Server[x].string);
    }

    if (debug) printf("All values stored.\n");

    free(Server); // Free dynamically allocated memory


    exit(1);

}

/* -------------------------------------------
  function: processMessage
  Parses '!' messages into their parts - 
  returns struct in response.
 ------------------------------------------- */
struct whiteboard processMessage(char * message)
{
    int lineNumber, numBytes;
    char stringType, entry[1028];

    if (debug) printf("Update Statement!\n");

    // Read line sent by Socket
    sscanf(message,"%*c%d%c%d\n%[^\n]s",&lineNumber,&stringType,&numBytes,entry);

    if (debug) printf("Processed: Line: %d, Text: %s\n",lineNumber,entry);

    // Parse information into local Struct
    struct whiteboard Server;
    Server.line = lineNumber;
    Server.type = stringType;
    Server.bytes = numBytes;
    strcpy(Server.string,entry);

    // If there is no bytes, give nothing
    if (numBytes == 0)
    {
        strcpy(Server.string,"");
    }

    return Server;
}


/* -------------------------------------------
  function: handleEverything
  Determines type of message recieved and 
  process and parses accordingly.
 ------------------------------------------- */
char * handleEverything(char* message, struct whiteboard *Server, char* newMessage)
{
    bool updateFlag = false, queryFlag = false;

    // If message is an Entry
    if (message[0] == '@')
    {
        if (debug) printf("Triggered Entry!\n");

        // Create Temporary Struct
        struct whiteboard messageReturn;
        messageReturn = processMessage(message);

        // Store Temporary Struct in Correct Heap Struct
        Server[messageReturn.line] = messageReturn;
        sprintf(newMessage,"!%d%c%d\n%s\n",messageReturn.line, messageReturn.type, messageReturn.bytes, messageReturn.string);
        return newMessage;
    }

    // If message is a query
    if (message[0] == '?')
    {
        if (debug) printf("Triggered Query!\n");

        int x;
        queryFlag = true;

        sscanf(message,"%*c%d",&x); // Parse Query

        if (x > serverSize) // Check if Query out of Range
        {
            strcpy(newMessage,"ERROR: Query out of Range.\n");
            return newMessage;
        }   

        sprintf(newMessage,"!%d%c%d\n%s\n",Server[x].line,Server[x].type,Server[x].bytes,Server[x].string);

        if (debug) printf("newMessage as of handleEverything:%s\n",newMessage);
        return newMessage;
    }   
}

/* -------------------------------------------
  function: readFile
  If argument -f given, read file
  process and parse into heap memory.
 ------------------------------------------- */
void readFile(char * filename)
{

    FILE *fp;
    fp=fopen(filename,"r");

    int line, bytes, count = 0, totalSize = 0;
    char type, check, string[1028], individualLine[1028];


    // Loop to determine size of file. **I know this is sloppy.
    while (fgets(individualLine, sizeof(individualLine), fp)) 
    {
        totalSize++;
    }

    // Each line shoud have totalSize - 2 (to account for 0)
    // (answer) / 2 to account for string line and instruction.
    totalSize = (totalSize - 2) / 2;
    serverSize = totalSize+1;
    if (debug) printf("Total Size is: %d\n",serverSize);

    // Open and Allocate Memory
    fp=fopen(filename,"r");
    if (debug) printf("File Mode Calloc Initialize\n");
    Server = calloc(serverSize+2, sizeof(*Server));

    // Write to Heap Loop
    while (fgets(individualLine, sizeof(individualLine), fp)) {
        if (individualLine[0] == '@') // Case of Header Line
        {               
            sscanf(individualLine,"%c%d%c%d",&check,&line,&type,&bytes);
            if (debug) printf("Count: %d, Check:%c, Line:%d, Type: %c, Bytes:%d \n",count,check,line,type,bytes);

            Server[count].line = line;
            Server[count].type = type;
            Server[count].bytes = bytes;
            count++;    
        }

        else
        {
            // For case of no data
            if (individualLine[0] == '\n')
            {
                strcpy(string,"");
            }

            // Then scan data line
            sscanf(individualLine,"%[^\n]s",string);        
            if (debug) printf("String: %s\n",string);           
            strcpy(Server[count-1].string,string);
        }
    }
    return;
}


void *threadFunction(int snew)
{
    char tempmessage[1024], message[2048];
    // Compile and Send Server Message
    strcpy(tempmessage, "CMPUT379 Whiteboard Server v0\n");
    send(snew, tempmessage, sizeof(tempmessage), 0);

    // Recieve Message
    char n = recv(snew, message, sizeof(message), 0);

    pthread_mutex_lock(&mutex);

    if (debug) printf("Attempt to Malloc for newMessage\n");
    char * newMessage = malloc(1024 * sizeof(char));

    if (debug) printf("goto: handleEverything\n");
    newMessage = handleEverything(message, Server, newMessage);
    if (debug) printf("returnMessage:%s\n",newMessage);

    strcpy(message,newMessage); 
    free(newMessage);

    pthread_mutex_unlock(&mutex);

    if (debug) printf("message = %s\n", message);

    send(snew, message, sizeof(message), 0);
    printf("End of threadFunction\n");

   return;
}



/* -------------------------------------------
  function: main
  Function Body of Server
 ------------------------------------------- */
int main(int argc, char * argv[])
{
  int sock, fromlength, outnum, i, socketNumber, snew;
  bool cleanMode;

  // Initialize Signal Handling
  struct sigaction act;
  act.sa_handler = sigint_handler;
  sigemptyset(&act.sa_mask);
  act.sa_flags = 0;
  sigaction(SIGINT, &act, 0);

  // For correct number of arguments.
  if (argc == 4)
  {
    // If "-n" parameter (cleanMode)
    if (strcmp(argv[2], "-n") == 0)
    {
        // Get size + 1
        cleanMode = true;
        sscanf(argv[3],"%d",&serverSize);
        serverSize += 1;

        if (debug) printf("== Clean Mode Properly Initiated == \n");
        if (debug) printf("serverSize: %d\n",serverSize);

        if (debug) printf("Clean Mode Calloc\n");
        Server = calloc(serverSize, sizeof(*Server));

        int i = 0;
        for (i; i < serverSize; i++) // Initialize allocated Memory
        {
            Server[i].line = i;
            Server[i].type = 'p';
            Server[i].bytes = 0;
            strcpy(Server[i].string,"");
        }
    }

    // If "-f" parameter (filemode)
    else if (strcmp(argv[2], "-f") == 0)
    {
        // Read File
        cleanMode = false;
        readFile(argv[3]);

        if (debug) printf("== Statefile Mode Properly Initiated == \n");
        if (debug) printf("serverSize: %d\n",serverSize);
    }

    // Otherwise incorrect parameter.
    else
    {
        printf("Incorrect Argument. \n");
        printf("Usage: wbs279 pornumber {-n number | -f statefile}\n");
        exit(1);
    }

    sscanf(argv[1],"%d",&socketNumber);
  }

  // Send Error for Incorrect Number of Arguments
  if (argc != 4)
  {
    printf("Error: Incorrect Number of Input Arguments.\n");
    printf("Usage: wbs279 portnumber {-n number | -f statefile}\n");
    exit(1);
  }

  // == Do socket stuff ==

  char tempmessage[1024], message[2048];
  struct sockaddr_in master, from;

  if (debug) printf("Assrt Socket\n");

  sock = socket (AF_INET, SOCK_STREAM, 0);
  if (sock < 0) 
  {
    perror ("Server: cannot open master socket");
    exit (1);
  }

  master.sin_family = AF_INET;
  master.sin_addr.s_addr = INADDR_ANY;
  master.sin_port = htons (socketNumber);

  if (bind (sock, (struct sockaddr*) &master, sizeof (master))) 
  {
    perror ("Server: cannot bind master socket");
    exit (1);
  }

  // == Done socket stuff == 

  listen (sock, 5);
  int threadNumber = 0;

  while(1) 
  {
    printf("But what about now.\n");
    if (debug) printf("-- Wait for Input --\n");
    printf("Enie, ");
    fromlength = sizeof (from);
    printf("Meanie, ");
    snew = accept (sock, (struct sockaddr*) & from, & fromlength);
    printf("Miney, ");  
    if (snew < 0) 
    {
      perror ("Server: accept failed");
      exit (1);
    }
    printf("Moe\n");


    pthread_create(&thread[threadNumber],NULL,threadFunction(snew), &id[threadNumber]);
    //printf("Can I join?!\n");
    //pthread_join(thread[0],NULL);
    //printf("Joined?!\n");

    threadNumber++;

    close (snew);
  }
}

I'm also curious as to how exactly to let multiple clients use the server at once. Is how I've allocated the whiteboard structure data appropriate for this process?

I'm very sorry if these don't make any sense.

Answer 1

This signal handler is completely unsafe:

void sigint_handler(int sig) 
{
    if (debug) printf("\nInduced SIGINT.\n");

    FILE *fp;
    fp=fopen("whiteboard.all","w");

    int x=0;
    for (x;x<serverSize;x++) // Loop Responsible for iterating all the whiteboard entries.
    {
        if (debug) printf("@%d%c%d\n%s\n",Server[x].line,Server[x].type,Server[x].bytes,Server[x].string);
        fprintf(fp,"@%d%c%d\n%s\n",Server[x].line,Server[x].type,Server[x].bytes,Server[x].string);
    }

    if (debug) printf("All values stored.\n");

    free(Server); // Free dynamically allocated memory


    exit(1);

}

Per 2.4.3 Signal Actions of the POSIX standard (emphasis added):

The following table defines a set of functions that shall be async-signal-safe. Therefore, applications can call them, without restriction, from signal-catching functions. ...

[list of async-signal-safe functions]

Any function not in the above table may be unsafe with respect to signals. Implementations may make other interfaces async-signal-safe. In the presence of signals, all functions defined by this volume of POSIX.1-2008 shall behave as defined when called from or interrupted by a signal-catching function, with the exception that when a signal interrupts an unsafe function or equivalent (such as the processing equivalent to exit() performed after a return from the initial call to main()) and the signal-catching function calls an unsafe function, the behavior is undefined. Additional exceptions are specified in the descriptions of individual functions such as longjmp().

Your signal handler invokes undefined behavior.

Answer 2

You seem to somehow expect this:

pthread_create(&thread[threadNumber],NULL,threadFunction(snew), &id[threadNumber]);
/* ... */
close (snew);

To make sense, while it clearly doesn't.

Instead of starting a thread that runs threadFunction, passing it snew, you call the thread function and pass the return value to pthread_create(), which will interpret it as a function pointer. This will break, especially considering that the thread function incorrectly ends with:

return;

This shouldn't compile, since it's declared to return void *.

Also assuming you managed to start the thread, passing it snew to use as its socket: then you immediately close that socket, causing any reference to it from the thread to be invalid!

Please note that pthread_create() does not block and wait for the thread to exit, that would be kind of ... pointless. It starts off the new thread to run in parallel with the main thread, so of course you can't yank the carpet away from under it.