In C, why does reading from a socket cause printw to give a segmentation fault?

Question

I'm writing a program that connects to a server, gets the sent data and displays it to the screen. I'm working on creating a UI for the program using ncurses. I initialize the screen, connect to the server, read the socket, print the output, close the socket, end the screen and this gives me a segmentation fault.

If I skip the socket read, no fault is given and the screen displays; however, there is a pause before it displays. I wonder if it has to do with the fact that my socket is non blocking?

I have 3 files, main.c socket.c and screen.c. I'll truncate the files for the needed information.

main.c

#include "socket.h"
#include "screen.h"

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

  socketConnect(argv[1], argv[2]);
  sleep(1);

  char buffer[1024];
  socketRead(buffer, sizeof buffer);//commenting this out lets the screen display
  screenDraw(buffer);

  socketClose();
  screenEnd();

  return 0;
}

socket.c

#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <netdb.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <fcntl.h>

int sockfd;

int socketRead(char *output, int length){
  memset(output, 0, length);

  char buffer[1024];
  memset(buffer, 0, sizeof buffer);

  for(;;){
    char *ch;

    if(recv(sockfd, ch, 1, 0) == -1)
      return -1;

    if(strlen(buffer)+1 == sizeof buffer)
      return -2;

    strcat(buffer, ch);
    if(*ch == '\n'){
      if(strlen(buffer)+1 <= length){
        strcpy(output, buffer);
        return 0;
      }else{
        return -3;
      }
    }
  }
}

int socketConnect(char *host, char *port){
  //sockfd gets set here
  fcntl(sockfd, F_SETFL, O_NONBLOCK);
  return 0;
}

void socketClose(){
  close(sockfd);
}

screen.c

#include <ncurses.h>

void screenInit(){
  initscr();
  cbreak();
  noecho();
}

void screenDraw(char *string){
  printw("%s", string);
  refresh();
  getch();
}

void screenEnd(){
  endwin();
}

Here's the stack from gdb

#0 0x00007ffff7647caa in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#1 0x00007ffff7649191 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#2 0x00007ffff764b580 in malloc() from /lib/x86_64-linux-gnu/libc.so.6
#3 0x00007ffff7bce2de in _nc_printf_string () from /lib/x86_64-linux-gnu/libncurses.so.5
#4 0x00007ffff7bc93ef in vwprintw () from /lib/x86_64-linux-gnu/libncurses.so.5
#5 0x00007ffff7bca0e9 in printw () from /lib/x86_64-linux-gnu/libncurses.so.5
#6 0x000000000040111f in screenDraw ()
#7 0x0000000000400d56 in main ()

I thought I'd add my Makefile here as well if for some reason that had something to do with the problem.

Makefile

objects = main.o socket.o screen.o

program: $(objects)
    cc -Wall -o program $(objects) -lncurses

.PHONY: clean
clean:
    @-rm -f program

Answer 1

You have an error in socketRead() here:

char *ch;

if(recv(sockfd, ch, 1, 0) == -1)
  return -1;

ch is only a pointer, you never allocated memory to store something in there - all you have is a pointer that was not initialized. You can't use it and pass it to recv, because recv will attempt to write to an undefined location. This is why your program crashes.

I think you want to use buffer here:

if (recv(sockfd, buffer, sizeof buffer, 0) == -1) { ... }

Or, alternatively, you can make ch point to a valid location before using it:

ch = buffer;
if (recv(sockfd, ch, sizeof buffer, 0) == -1) { ... }

Note, however, that in this case you can't use sizeof ch, since that would yield the size of a char *.

If you want to explicitly leave out space for a null terminating byte, then use sizeof(buffer) - 1:

if (recv(sockfd, buffer, sizeof buffer, 0) == -1) { ... }

These lines are also wrong, for the same reason: you never initialized ch

strcat(buffer, ch);
if (*ch == '\n') {

Note that recv(sockfd, buffer, sizeof(buffer)-1, 0) will read chunks of at most sizeof(buffer)-1 bytes. Your code conveys the idea that you want to read it char by char; in that case, you probably want to make ch a char instead of a char *. In that case, you just have to use the address of operator in the call to recv, because recv expects a pointer:

if (recv(sockfd, &ch, 1, 0) == -1)

But then of course, you can't use strcat with a char (strcat expects null terminated sequences of characters), and instead of if (*ch == '\n'), you must use if (ch == '\n').

Answer 2

The char pointer is not initialised to the buffer:

char *ch = buffer;
if(recv(sockfd, ch, 1, 0) == -1)
  return -1;

Error handling is also needed to read the return code from SocketRead() - perhaps you left that out of your post for brevity.