Working with sockets in C

Question

I am trying to create a basic client server socket program for a chat room. Currently it sends the first message to the server as a login and prints off the right stuff but the client crashes after that and won't let me send another message to the server. Below is the code

Client code

#include <stdio.h>
#include "winsock2.h"

#define SERVER_PORT  9999
#define MAX_LINE      256
void main(int argc, char **argv) {

  if (argc < 2){
     printf("\nUseage: client serverName\n");
     return;
  }

// Initialize Winsock.
  WSADATA wsaData;
  int iResult = WSAStartup( MAKEWORD(2,2), &wsaData );
  if ( iResult != NO_ERROR ){
     printf("Error at WSAStartup()\n");
     return;
  }

//translate the server name or IP address (128.90.54.1) to resolved IP address
  unsigned int ipaddr;
// If the user input is an alpha name for the host, use gethostbyname()
// If not, get host by addr (assume IPv4)
  if (isalpha(argv[1][0])) {   // host address is a name  
     hostent* remoteHost = gethostbyname(argv[1]);
     if ( remoteHost == NULL){
        printf("Host not found\n");
        WSACleanup(); 
        return;
     }
     ipaddr = *((unsigned long *) remoteHost->h_addr);
  }
  else //"128.90.54.1"
     ipaddr = inet_addr(argv[1]);


// Create a socket.
  SOCKET s;

// Connect to a server.
  sockaddr_in addr;
  addr.sin_family = AF_INET;
  addr.sin_addr.s_addr = ipaddr;
  addr.sin_port = htons( SERVER_PORT );
  s = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
  if (s == INVALID_SOCKET) {
      printf("Error at socket(): %ld\n", WSAGetLastError());
      WSACleanup();
      return;
  }
  if (connect(s, (SOCKADDR*)&addr, sizeof(addr)) == SOCKET_ERROR) {
      printf("Failed to connect.\n");
      WSACleanup();
      return;
  }

  while (1) {

      // Send and receive data.
      char buf[MAX_LINE];
      printf("Type whatever you want: ");
      scanf("%[^\n]", buf);
      send(s, buf, strlen(buf), 0);
      char recieve[MAX_LINE];
      int len = recv(s, recieve, MAX_LINE, 0);
      recieve[len] = 0;
      printf("Server says: %s\n", recieve);
      closesocket(s);
  }



}

Server code

#include <stdio.h>
#include "winsock2.h"

#define SERVER_PORT   9999
#define MAX_PENDING   5
#define MAX_LINE      256
#define MAX_USERS 10

char users[MAX_USERS][MAX_LINE];
char passwords[MAX_USERS][MAX_LINE];
int count = 0;
int loggedin = 0;
char* Login(char*, char*);
char* username = NULL;
void main() {

    // Initialize Winsock.
    WSADATA wsaData;
    int iResult = WSAStartup(MAKEWORD(2, 2), &wsaData);
    if (iResult != NO_ERROR) {
        printf("Error at WSAStartup()\n");
        return;
    }

    // Create a socket.
    SOCKET listenSocket;
    listenSocket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);

    if (listenSocket == INVALID_SOCKET) {
        printf("Error at socket(): %ld\n", WSAGetLastError());
        WSACleanup();
        return;
    }

    // Bind the socket.
    sockaddr_in addr;
    addr.sin_family = AF_INET;
    addr.sin_addr.s_addr = INADDR_ANY; //use local address
    addr.sin_port = htons(SERVER_PORT);
    if (bind(listenSocket, (SOCKADDR*)&addr, sizeof(addr)) == SOCKET_ERROR) {
        printf("bind() failed.\n");
        closesocket(listenSocket);
        WSACleanup();
        return;
    }

    // Listen on the Socket.
    if (listen(listenSocket, MAX_PENDING) == SOCKET_ERROR) {
        printf("Error listening on socket.\n");
        closesocket(listenSocket);
        WSACleanup();
        return;
    }

    // Accept connections.
    SOCKET s;
    FILE* file = fopen("C:\\Users\\Brandon\\Desktop\\server2\\users.txt", "r");
    if (file == NULL) {
        printf("file didnt open");
        exit(1);
    }
    char* user = (char*)malloc(MAX_LINE);
    char* password = (char*)malloc(MAX_LINE);
    int i = 0;
    while (fscanf(file, "%s %s", user, password) != EOF) {
        strcpy(users[i], user);
        strcpy(passwords[i], password);
        i++;
    }
    count = i;
    printf("Waiting for a client to connect...\n");
    while (s = accept(listenSocket, NULL, NULL)) {
        if (s == SOCKET_ERROR) {
            printf("accept() error \n");
            closesocket(listenSocket);
            WSACleanup();
            return;
        }
        // Send and receive data.
        char buf[MAX_LINE];
        char buf1[MAX_LINE];
        int len = recv(s, buf, MAX_LINE, 0);
        buf[len] = 0;
        char* type = strtok(buf, " ");
        char* username = strtok(NULL, " ");
        char* password1 = strtok(NULL, " ");
        if (strcmp(type, "login") == 0) {
            strcpy(buf1, Login(username, password1));
        }
        send(s, buf1, strlen(buf1), 0);
        closesocket(s);
        printf("Client Closed.\n");
    }

    closesocket(listenSocket);
}

char* Login(char* username1, char* password) {
    for (int i = 0; i < count; i++) {
        if (strcmp(users[i], username1) == 0 && strcmp(passwords[i], password) == 0) {
            printf("%s logged in.\n", username1);
            loggedin = 1;
            username = username1;
            return strcat(username1, " Has logged in\n");
        }
    }
    return "User name and password are incorrect";
}

char* Logout() {
    printf("%s logout.", username);
    loggedin = 0;
    return strcat(username, " left");
}

Answer 1

I figured this out, scanf inside the loop on the client wasn't working correctly in handling the space, I also cleared out the buffer arrays each time

Correct Client Loop

  while (s = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)) {
      if (s == INVALID_SOCKET) {
          printf("Error at socket(): %ld\n", WSAGetLastError());
          WSACleanup();
          return;
      }
      if (connect(s, (SOCKADDR*)&addr, sizeof(addr)) == SOCKET_ERROR) {
          printf("Failed to connect.\n");
          WSACleanup();
          return;
      }
          // Send and receive data.
          char buf[MAX_LINE] = "";
          printf("Type whatever you want: ");
          scanf(" %[^\n]", buf);
          send(s, buf, strlen(buf), 0);
          char recieve[MAX_LINE]= "";
          int len = recv(s, recieve, MAX_LINE-1, 0);
          recieve[len] = 0;
          printf("Server says: %s\n", recieve);
      }
      closesocket(s);

Correct server code

while (s = accept(listenSocket, NULL, NULL)) {
        if (s == SOCKET_ERROR) {
            printf("accept() error \n");
            closesocket(listenSocket);
            WSACleanup();
            return;
        }
        // Send and receive data.
        char buf[MAX_LINE] = "";
        char buf1[MAX_LINE] = "";
        int len = recv(s, buf, MAX_LINE-1, 0);
        buf[len] = 0;
        char* type = strtok(buf, " ");
        char* username = strtok(NULL, " ");
        char* password1 = strtok(NULL, " ");
        if (strcmp(type, "login") == 0) {
            strcpy(buf1, Login(username, password1));
        }
        send(s, buf1, strlen(buf1), 0);
        closesocket(s);

    }

Answer 2

This: 'buf[len] = 0;' writes out-of bounds if MAX_LINE chars are loaded by the recv() call. Call recv() with MAX_LINE-1 to prevent that. Both client and server have this problem.

The parsing of 'buf' with strtok etc. is not secure. There is no guarantee that complete application-level messages are loaded by one call to recv(). TCP cannot transfer messages larger than one byte. If you want to transfer strings, you need a protocol on top, eg, repeated calls until some end-of-message char, (eg. a newline or NUL), is received.