Can you assess source code of my client-server program?I would like to get an advice about design, code security etc. Thank you very much.The program does the following:Show files in directories; Download and upload files (for now, only one file at a time).Syntax:./client <Server IP> <Server PORT> dir <PATH> ./client <Server IP> <Server PORT> upload localfile.txt remotefile.txt./client <Server IP> <Server PORT> download remotefile.txt localfile.txtClient source code:client.c#include <stdio.h>#include <sys/socket.h>#include <arpa/inet.h>#include <stdlib.h>#include <string.h>#include <unistd.h>#define BUFFSIZE 4096 // Size of receive bufferint parseARGS(char **args, char *line);void DieWithError(char *errorMessage); // Error handling functionvoid UploadFile(int sock, char *lfile, char *rfile);void DownloadFile(int sock, char *lfile, char *rfile);void SysCmd(int sock, char *myCommand, char *myArgs);void ShowExamples(void);int main(int argc, char *argv[]){   int i, percent_sent, sock;                      // Socket descriptor   struct sockaddr_in servAddr;  // Echo server address   unsigned short servPort;      // Echo server port   char *servIP;                    // Server IP addr   char *myCommand, *myArgs, *commandOpt, *lfile, *rfile;   // Check cmd args   if(argc == 6 || argc == 5){      servIP = argv[1];                // Server IP addr      servPort = atoi(argv[2]);      myCommand = argv[3];        if(!strcmp(myCommand, dir))            myArgs = argv[4];      //if(argc == 6){        if(!strcmp(myCommand, upload) || !strcmp(myCommand, download)){         lfile = argv[4];         rfile = argv[5];      }   } else {        ShowExamples();      exit(1);   }   // Create a reliable, stream socket using TCP   if((sock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0)      DieWithError(socket() failed!);   // Construct the server address structure   memset(&servAddr, 0, sizeof(servAddr));   // Zero out structure   servAddr.sin_family = AF_INET;                  // Internet addr family   servAddr.sin_addr.s_addr = inet_addr(servIP);   // Server IP addr. inet_addr transforms IP addr to bin format   servAddr.sin_port = htons(servPort);         // Server port. htons make sure bytes are stored in big endian    // Establish connection to echo server   if(connect(sock, (struct sockaddr*) &servAddr, sizeof(servAddr)) < 0)      DieWithError(connect() failed!);   // Select a cmd command   if(!strcmp(myCommand, upload) && argc == 6)      UploadFile(sock, lfile, rfile);   else if(!strcmp(myCommand, download) && argc == 6)      DownloadFile(sock, lfile, rfile);   else if(!strcmp(myCommand, dir) && argc == 5)      SysCmd(sock, myCommand, myArgs);   else        ShowExamples();   // Close the socket   close(sock);   exit(0);}void ShowExamples(void){    printf(Available commands: dir, upload, download.\nExamples:\n-----\n);    printf(./client <Server IP> <Server PORT> dir <PATH>\n);    printf(./client <Server IP> <Server PORT> upload localfile.txt remotefile.txt\n);    printf(./client <Server IP> <Server PORT> download remotefile.txt localfile.txt\n-----\n);}void DieWithError(char *errorMsg){   perror(errorMsg);   exit(1);}void UploadFile(int sock, char *lfile, char *rfile){     int percent_sent, chunk_size = 256;   long all_bytes_sent, bytes_sent, bytes_read, bytes_left, file_size;   char buffer[BUFFSIZE];      // Buffer for echo string   FILE *aFile;   // Open file for read in binary mode   aFile = fopen(lfile, rb);   if(!aFile)      DieWithError(fopen() failed!\n);   // Set pos indicator to the END of file    // and return curr pos         fseek(aFile, 0, SEEK_END);   file_size = ftell(aFile);   rewind(aFile);   bytes_left = file_size;   // Init buffer with 0s   // then fill it with file data   memset(buffer, 0, sizeof(buffer));   sprintf(buffer, UPLOAD:%s:%ld\r\n, rfile, file_size);       // Send UPLOAD stat msg with file name and size.    // Repeat until amount of all sent Bytes equals 4096 Bytes   all_bytes_sent = 0;   while(all_bytes_sent != sizeof(buffer)){      bytes_sent = send(sock, (buffer + all_bytes_sent), (sizeof(buffer) - all_bytes_sent), 0);      if(bytes_sent < 0)         DieWithError(send() UPLOAD msg failed!\n);      all_bytes_sent += bytes_sent;   }   //// Allocate dyn memmory to store file before sending    //char *file_buffer = (char *)malloc(chunk_size * sizeof(char));     //if(NULL == file_buffer){   // fprintf(stderr, malloc failed\n);   // return(-1);   //}   printf(\n-----\n);   // Loop until all Bytes of the file will be send   while(1){      char file_buffer[chunk_size];      memset(file_buffer, 0, sizeof(file_buffer));      // Read file to file_buffer      if((bytes_read = fread(file_buffer, sizeof(char), chunk_size, aFile)) < 0)         DieWithError(fread() failed!\n);      // Send file over socket      if(bytes_read > 0){         if((bytes_sent = send(sock, file_buffer, bytes_read, 0)) < 0)            DieWithError(send() after fread() failed!);         // Calc percentage and display status         bytes_left -= bytes_sent;         percent_sent = ((file_size - bytes_left) * 100) / file_size;         printf(Sent %d%% (%ld B), remaining = %ld B\n, percent_sent, bytes_sent, bytes_left);      }      // Check the end of file      // and if it is the end - break loop      if(bytes_read < chunk_size){         if(feof(aFile))            printf(End of file.\n);         if(ferror(aFile))            printf(Error reading!\n);         break;      }   }   // Close the file   fclose(aFile);   //free(file_buffer);   printf(\n);}void DownloadFile(int sock, char *lfile, char *rfile){   //printf(Download func is under construction!\n);   //exit(1);    int percent_recvd;   long all_bytes_sent, bytes_sent, bytes_read, bytes_left, file_size;   long all_bytes_recvd, bytes_recvd, bytes_written;   char buffer[BUFFSIZE];      // Buffer for echo string    char *file_name, *header[BUFFSIZE];   FILE *aFile;    // Create status message for DOWNLOAD    memset(buffer, 0, sizeof(buffer));    sprintf(buffer, DOWNLOAD:%s:%d\r\n, lfile, 1);    // Send status msg to request file for DOWNLOAD.    // Repeat until amount of all received Bytes equals 4096 Bytes.    all_bytes_sent = 0;   while(all_bytes_sent != sizeof(buffer)){      bytes_sent = send(sock, (buffer + all_bytes_sent), (sizeof(buffer) - all_bytes_sent), 0);      if(bytes_sent < 0)         DieWithError(send() DOWNLOAD msg failed!\n);      all_bytes_sent += bytes_sent;   }    // Receive stat message to request file size for DOWNLOAD.    // Repeat until amount of all received Bytes equals 4096 Bytes.    memset(buffer, 0, sizeof(buffer));    all_bytes_recvd = 0;   while(all_bytes_recvd != sizeof(buffer)){      bytes_recvd = recv(sock, (buffer + all_bytes_recvd), (sizeof(buffer) - all_bytes_recvd), 0);      if(bytes_recvd < 0)         DieWithError(recv() STAT msg failed!\n);      if(bytes_recvd == 0){         printf(Received STAT msg!\n);            break;        }      all_bytes_recvd += bytes_recvd;      //printf(STAT: Received %ld B, remaining data = %ld B\n, bytes_recvd, (sizeof(buffer) - all_bytes_recvd));   }    // Parse received stat msg to get file name and size    parseARGS(header, buffer);      //file_name = header[1];                        // It is not used. Instead rfile used.    file_size = atoi(header[2]);    bytes_left = file_size;    // Open file stream to write in bin mode    aFile = fopen(rfile, wb);    if(aFile == NULL)        DieWithError(failed to open the file!\n);    // Receive file via socket, place it in 4096 Byte array     // than write buffer content into file.    // Repeat until amount of all received Bytes equals file size.     memset(buffer, 0, sizeof(buffer));    all_bytes_recvd = 0;    while(all_bytes_recvd != file_size){        bytes_recvd = recv(sock, buffer, sizeof(buffer), 0);        if(bytes_recvd < 0)            DieWithError(failed to receive the file!\n);              if((bytes_written = fwrite(buffer, sizeof(char), bytes_recvd, aFile)) < 0)            DieWithError(failed to write into the file!\n);        all_bytes_recvd += bytes_recvd;        //printf(Received %ld B, remaining data = %ld B\n, bytes_recvd, (file_size - all_bytes_recvd));      // Calc percentage and display status      bytes_left -= bytes_recvd;      percent_recvd = ((file_size - bytes_left) * 100) / file_size;      printf(Received %d%% (%ld B), remaining = %ld B\n, percent_recvd, bytes_recvd, bytes_left);    }       // Close file stream    fclose(aFile);}// EXEC commandsvoid SysCmd(int sock, char *myCommand, char *myArgs){    int i = 0;    long bytes_recvd, all_bytes_recvd, bytes_sent, all_bytes_sent;    char buffer[BUFFSIZE], *header[BUFFSIZE];    // Create status message for EXEC command    memset(buffer, 0, sizeof(buffer));    sprintf(buffer, EXEC:%s:%s:1\r\n, myCommand, myArgs); // 1 - to delimit args from \r\n    // Send the status message    all_bytes_sent = 0;    while(all_bytes_sent != sizeof(buffer)){        if((bytes_sent = send(sock, (buffer + all_bytes_sent), (sizeof(buffer) - all_bytes_sent), 0)) < 0)            DieWithError(send() failed!\n);        all_bytes_sent += bytes_sent;    }    memset(buffer, 0, sizeof(buffer));    // Receive response for EXEC command    all_bytes_recvd = 0;    while(all_bytes_recvd != sizeof(buffer)){        if((bytes_recvd = recv(sock, (buffer + all_bytes_recvd), (sizeof(buffer) - all_bytes_recvd), 0)) < 0)            DieWithError(recv() failed!\n);        if(bytes_recvd < 0)            DieWithError(failed to receive the message from server!\n);               all_bytes_recvd += bytes_recvd;    }    memset(header, 0, BUFFSIZE);    // Parse buffer to copy members to header array     parseARGS(header, buffer);    // Print output    while(header[i] != NULL){        printf(%s\n, header[i]);        i++;    }    // Clean both arrays    memset(buffer, 0, sizeof(buffer));    memset(header, 0, BUFFSIZE);}int parseARGS(char **args, char *line){   int tmp = 0;    // Parse line to get args[] elements by : delimeter   args[tmp] = strtok(line, :);   while ((args[++tmp] = strtok(NULL, :)) != NULL);   return tmp - 1;}Server source code:server.c#include lib/TCPServer.h#include <pthread.h>void *ThreadMainRoutine(void *arg);         // Main program of threadstruct ThreadArgs                           // Structure of args to pass to client thread {    int clntSock;};int main(int argc, char *argv[]){    int servSock, clntSock;             // Sockets descriptors    unsigned    short servPort;     // Server port    pthread_t threadID;                 // Thread ID    struct ThreadArgs *threadArgs;    if(argc != 2){                          // Test for correct number of args        fprintf(stderr, Usage %s <Server Port>\n, argv[0]);        exit(1);    }    servPort = atoi(argv[1]);    servSock = CreateTCPServerSocket(servPort);    for(;;){        clntSock = AcceptTCPConnection(servSock);        // Create separate memory for client arg        if((threadArgs = (struct ThreadArgs*) malloc(sizeof(struct ThreadArgs))) == NULL)            DieWithError(malloc() failed!);        threadArgs -> clntSock = clntSock;        // Create client thread        if(pthread_create(&threadID, NULL, ThreadMainRoutine, (void *) threadArgs) != 0)            DieWithError(pthread_create() failed!);        printf(with thread %ld\n, (long int) threadID);    }}void *ThreadMainRoutine(void *threadArgs){    int clntSock;    // Thread resources are deallocated upon return    pthread_detach(pthread_self());    //  Extract socket descriptor from arg    clntSock = ((struct ThreadArgs*) threadArgs) -> clntSock;    free(threadArgs);    HandleTCPClient(clntSock);    return (NULL);}TCPServer.h#include <stdio.h>#include <sys/socket.h>#include <arpa/inet.h>  // for sockaddr_in and inet_ntoa()#include <stdlib.h>     // for atoi()#include <string.h>     //  for memset#include <unistd.h>     //  for clode()#include <string.h>#include <netdb.h>#include <netinet/in.h>void DieWitError(char *errMSG);                     // Error handling functionvoid HandleTCPClient(int clntSocket);               // TCP client handling void HandleTCPClientFull(int clntSocket);               // TCP client handling int CreateTCPServerSocket(unsigned short port); // Create TCP server socket int AcceptTCPConnection(int servSock);              // Accept TCP connection reqCreateTCPServerSocket.c#include <sys/socket.h>#include <arpa/inet.h>#include <string.h>#define MAXPENDING 5                    // Max outstanding connection reqvoid DieWithError(char *errMsg);int CreateTCPServerSocket(unsigned short port){    int sock;    struct sockaddr_in servAddr;    // Create socket for incomming connection    if((sock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0)        DieWithError(socket() failed!);    // Construct local addr structure    memset(&servAddr, 0, sizeof(servAddr));         // Zero out structure    servAddr.sin_family = AF_INET;                      // Internet addr family    servAddr.sin_addr.s_addr = htonl(INADDR_ANY);   // Any incomming interface    servAddr.sin_port = htons(port);                        //  Local port    // Bind to local addr    if(bind(sock, (struct sockaddr*) &servAddr, sizeof(servAddr)) < 0)        DieWithError(bind() failed!);    // Mark the socket to listen for incomming connections    if(listen(sock, MAXPENDING) < 0)        DieWithError(listen() failed!);    return sock;}AcceptTCPConnection.c#include <stdio.h>#include <sys/socket.h>#include <arpa/inet.h>void DieWithError(char *errMsg);int AcceptTCPConnection(int servSock){    int clntSock;    struct sockaddr_in clntAddr;    unsigned int clntLen;    // Set size of the in-out parameter    clntLen = sizeof(clntAddr);    // Wait for a client to connect    if((clntSock = accept(servSock, (struct sockaddr*) &clntAddr, &clntLen)) < 0)        DieWithError(accept() failed!);    // clntSock is connected to a client    printf(Handling client %s\n, inet_ntoa(clntAddr.sin_addr));    return clntSock; }DieWithError.c#include <stdio.h>void DieWithError(char *errMsg){    printf(%s\n, errMsg);}HandleTCPClient.c#include <stdio.h>#include <stdlib.h>#include <string.h>#include <unistd.h>#include <arpa/inet.h>#include <sys/socket.h>#include <sys/types.h>#include <dirent.h>#define BUFFSIZE 4096int parseARGS(char **args, char *line);void UploadFile(int clntSock, char *buffer);void DownloadFile(int clntSock, char *buffer);void SysCmd(int clntSock, char *buffer);void HandleTCPClient(int clntSock){    char buffer[BUFFSIZE];    long bytes_recvd, all_bytes_recvd;    // Get initital client stat msg.    // Repeat until amount of all received Bytes equals 4096 Byte.    memset(buffer, 0, sizeof(buffer));    all_bytes_recvd = 0;    while(all_bytes_recvd != sizeof(buffer)){        bytes_recvd = recv(clntSock, (buffer + all_bytes_recvd), (sizeof(buffer) - all_bytes_recvd), 0);        if(bytes_recvd < 0)            DieWithError(recv() STAT msg failed!\n);        if(bytes_recvd == 0){            printf(Received STAT msg!\n);            break;        }        all_bytes_recvd += bytes_recvd;        printf(STAT: Received %ld B, remaining data = %ld B\n, bytes_recvd, (sizeof(buffer) - all_bytes_recvd));    }    // Read first characters of stat msg to determine action:    // Upload, Download or Exec.    if(!strncmp(buffer, UPLOAD, 6))        UploadFile(clntSock, buffer);    else if(!strncmp(buffer, DOWNLOAD, 8))        DownloadFile(clntSock, buffer);    else if(!strncmp(buffer, EXEC, 4))        SysCmd(clntSock, buffer);    else        printf(Wrong STAT message!\n);}int parseARGS(char **args, char *line){   int tmp=0;    // Parse line to get args[] elements by : delimeter   args[tmp] = strtok( line, : );   while ( (args[++tmp] = strtok(NULL, : ) ) != NULL );   return tmp - 1;}void UploadFile(int clntSock, char *buffer){    char *file_name, *header[BUFFSIZE];    long bytes_recvd, bytes_written, file_size, all_bytes_recvd;    FILE *aFile;    // Parse received buffer for file name and file size    parseARGS(header, buffer);    file_name = header[1];    file_size = atoi(header[2]);    // Open a file stream in wite bin mode    aFile = fopen(file_name, wb);    if(aFile == NULL)        DieWithError(failed to open the file!\n);   // Receive file via socket, place it in 4096 Byte array    // than write buffer content into file.   // Repeat until amount of all received Bytes equals file size.     memset(buffer, 0, BUFFSIZE);    all_bytes_recvd = 0;    while((bytes_recvd = recv(clntSock, buffer, BUFFSIZE, 0)) > 0){        all_bytes_recvd += bytes_recvd;        printf(Received %ld B, remaining data = %ld B\n, bytes_recvd, (file_size - all_bytes_recvd));        if((bytes_written = fwrite(buffer, sizeof(char), bytes_recvd, aFile)) < 0)            DieWithError(fwrite() failed!\n);    }    // Close file stream    fclose(aFile);  }void DownloadFile(int clntSock, char *sbuffer){    int percent_sent;    long bytes_sent, bytes_read, file_size, all_bytes_sent, bytes_left;    char buffer[BUFFSIZE], *file_name, *header[BUFFSIZE];    FILE *aFile;    // Parse buffer for file name     parseARGS(header, sbuffer);    file_name = header[1];    // Open file stream in read bin mode    aFile = fopen(file_name, rb);    if(aFile == NULL)        DieWithError(failed to open the file!\n);    // Shift file stream indicatore to get file size    fseek(aFile, 0, SEEK_END);    file_size = ftell(aFile);    rewind(aFile);    bytes_left = file_size;    // Prepare DOWNLOAD stat msg with file name and size        memset(buffer, 0, BUFFSIZE);    sprintf(buffer, DOWNLOAD:%s:%ld\r\n, file_name, file_size);    // Send DOWNLOAD stat msg via socket.    // Repeat until amount of all sent Bytes equals 4096 Bytes.     all_bytes_sent = 0;    while(all_bytes_sent != BUFFSIZE){        bytes_sent = send(clntSock, (buffer + all_bytes_sent), (BUFFSIZE - all_bytes_sent), 0);        if(bytes_sent < 0)            DieWithError(send STAT msg failed!\n);                all_bytes_sent += bytes_sent;        printf(STAT: Sent %ld B, remaining data = %ld B\n, bytes_sent, (BUFFSIZE - all_bytes_sent));    }    // Loop until all Bytes of the file will be send    while(1){        memset(buffer, 0, BUFFSIZE);        // Read file into buffer        if((bytes_read = fread(buffer, sizeof(char), BUFFSIZE, aFile)) < 0)            DieWithError(failed to read the file!\n);         // Send file over socket        if(bytes_read > 0){            if((bytes_sent = send(clntSock, buffer, bytes_read, 0)) < 0)                DieWithError(fialed to send the file!\n);            // Calc percentage and display status            bytes_left -= bytes_sent;            percent_sent = ((file_size - bytes_left) * 100) / file_size;            printf(Sent %d%% (%ld B), remaining = %ld B\n, percent_sent, bytes_sent, bytes_left);        }        // Check the end of the file        // if it is End - break.        if(bytes_read < BUFFSIZE){            if(feof(aFile))                printf(End of file.\n);            if(ferror(aFile))                printf(Error reading!\n);            break;        }    }    // Close file stream    fclose(aFile);  }void SysCmd(int clntSock, char *buffer){    char local_buffer[BUFFSIZE], *cmd_name, *cmd_args, *file_name, *header[BUFFSIZE];    long bytes_sent, all_bytes_sent;    DIR *dir;    struct dirent *dp;    printf(%s\n, buffer);    memset(local_buffer, 0, sizeof(local_buffer));    // Parse buffer to get EXEC commands    parseARGS(header, buffer);    cmd_name = header[1];    cmd_args = header[2];    printf(Args: %s\n, cmd_args);    // Execute command dir    if(!strcmp(cmd_name, dir)){        // Open directory        dir = opendir(cmd_args);        if(!dir)            DieWithError(opendir() failed!\n);        else {            // Read the directory and copy all file/dir names to local_buffer array            while((dp = readdir(dir)) != NULL) {                if (!strcmp(dp->d_name, .) || !strcmp(dp->d_name, ..)){                    // do nothing (straight logic)                } else {                   file_name = dp->d_name; // use it                        strcat(local_buffer, file_name);                        strcat(local_buffer, :);                }               }            // Server status msg            printf(%s\n, local_buffer);            // Send info about files/dirs to client.            // Repeat while amount of all sent Bytes equals BUFFSIZE.             all_bytes_sent = 0;            while(all_bytes_sent != sizeof(local_buffer)){                if((bytes_sent = send(clntSock, (local_buffer + all_bytes_sent), (sizeof(local_buffer) - all_bytes_sent), 0)) < 0)                      DieWithError(send() failed!\n);                all_bytes_sent += bytes_sent;               }        }        // Close directory        closedir(dir);    }}
Assessment of client-server program source code
c;linux;socket
Buffer overflowThe first thing I checked was for buffer overflow and I found it immediately.  On the client side in SysCmd():char buffer[BUFFSIZE], *header[BUFFSIZE];// Create status message for EXEC commandmemset(buffer, 0, sizeof(buffer));sprintf(buffer, EXEC:%s:%s:1\r\n, myCommand, myArgs); // 1 - to delimit args from \r\nHere, myArgs came from argv[4] and could be anything.  So if myArgs is longer than BUFFSIZE (4 KB), you will overflow your buffer.  You need to either use snprintf or check the length of your arguments before constructing the buffer.I spotted several calls to sprintf in your code and all of them have the same problem.Server side processingOn the server side, you have problems when you parse the incoming buffer sent from the client.  Remember, you have no idea what the contents of the buffer are because it could have been sent by a malicious program masquerading as a client.  So for example, you have this code in SysCmd() on the server side:    // Parse buffer to get EXEC commands    parseARGS(header, buffer);    cmd_name = header[1];    cmd_args = header[2];and this is the code for parseARGS():int parseARGS(char **args, char *line){   int tmp=0;    // Parse line to get args[] elements by : delimeter   args[tmp] = strtok( line, : );   while ( (args[++tmp] = strtok(NULL, : ) ) != NULL );   return tmp - 1;}If the client sent a buffer with too few arguments (something with no colons, for example), the calling function will end up dereferencing an argument string that doesn't exist.  In other words header[1] and header[2] in the example above may be NULL, and your server will crash.If the client sent a buffer with no null terminating character (for example, 4KB of all 'a'), then parseARGS will keep using strtok past the end of the buffer.  This can possibly cause it to write a 0 to memory if it finds a ':' character somewhere.  It might also cause your program to crash if it manages to read past the end of the stack.Handling uploads on the server sideOn the server side, when you handle an upload, I saw that you parse the file size passed by the client, but you don't actually use that size later on.  Your code for downloading the file is this:file_size = atoi(header[2]);  // <-- This isn't used!//// ... more code ...//while((bytes_recvd = recv(clntSock, buffer, BUFFSIZE, 0)) > 0){    all_bytes_recvd += bytes_recvd;    printf(Received %ld B, remaining data = %ld B\n, bytes_recvd, (file_size - all_bytes_recvd));    if((bytes_written = fwrite(buffer, sizeof(char), bytes_recvd, aFile)) < 0)        DieWithError(fwrite() failed!\n);}The problem here is that the client could send the file, and then immediately send another command.  But because you just keep reading BUFFSIZE chunks of data from the client, you might read a chunk that includes the end of the file plus the next command.  For example, the server might read this:This is the end of the file.EXEC:dir:/tmpand think that the whole thing is part of the file being uploaded instead of the end of the file plus another command.  You could use the file_size variable to make sure you don't read past the end of the file.4 KB MessagesRight now you use 4 KB fixed length messages.  On the one hand, having fixed length messages makes your code simpler in many respects.  On the other hand, you have to send 4 KB even if your command is EXEC:dir:/tmp or something small like that.It wouldn't be too hard switch to variable length messages by having all your messages begin with a 4 byte length followed by the rest of the message.  You could still enforce a maximum of 4 KB per message so that you could keep using fixed length buffers in your program.