1

I have the following code.

This code is for a TFTP server that creates a fork or a thread for each request that is receives. My problem is in the thread methods.

E.g I request 30 files from the server, is should creat 30 threads and give the requested files to the client, each thread will send each file. The code works perfectly if I use pthread_join (which is commented) but if I don't have pthread_join it serves some files correctly but some of them are corrupt or empty.

I believe this is a sync problem so I tried to malloc a piece of memory for each client filedescriptor so a thread can only modify it's own filedescriptor but with no luck. The following code, as is, works and serves some

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <string.h>
#include <fcntl.h>
#include <signal.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <sys/time.h>
#include <sys/stat.h>
#include <dirent.h>
#include <pthread.h>

#define BUFSIZE 8096
#define OperationMode 1

#if OperationMode
    typedef struct {
        int * fd;
        int hit;
    } THREAD_ARGS;

    void *attendFTP(void *);
#endif

int ftp(int fd, int hit);

void getFunction(int fd, char * fileName);

void putFunction(int fd, char * fileName);

char * listFilesDir(char * dirName);

void lsFunction(int fd, char * dirName);

void mgetFunction(int fd, char *dirName);

/* just checks command line arguments, setup a listening socket and block on accept waiting for clients */

int main(int argc, char **argv) {
    int i, port, pid, listenfd, socketfd, hit;
    socklen_t length;
    static struct sockaddr_in cli_addr; /* static = initialised to zeros */
    static struct sockaddr_in serv_addr; /* static = initialised to zeros */

    if (argc < 3 || argc > 3 || !strcmp(argv[1], "-?")) {
        printf("\n\nhint: ./tftps Port-Number Top-Directory\n\n""\ttftps is a small and very safe mini ftp server\n""\tExample: ./tftps 8181 ./fileDir \n\n");
        exit(0);
    }

    if (chdir(argv[2]) == -1) {
        printf("ERROR: Can't Change to directory %s\n", argv[2]);
        exit(4);
    }

    printf("LOG tftps starting %s - pid %d\n", argv[1], getpid());

    /* setup the network socket */
    if ((listenfd = socket(AF_INET, SOCK_STREAM, 0)) < 0)
        printf("ERROR system call - setup the socket\n");
    port = atoi(argv[1]);
    if (port < 0 || port > 60000)
        printf("ERROR Invalid port number (try 1->60000)\n");


    serv_addr.sin_family = AF_INET;
    serv_addr.sin_addr.s_addr = htonl(INADDR_ANY);
    serv_addr.sin_port = htons(port);

    if (bind(listenfd, (struct sockaddr *) &serv_addr, sizeof(serv_addr)) < 0)
        printf("ERROR system call - bind error\n");
    if (listen(listenfd, 64) < 0)
        printf("ERROR system call - listen error\n");


    // Main LOOP
    for (hit = 1 ;; hit++) {
        length = sizeof(cli_addr);

        /* block waiting for clients */
        socketfd = accept(listenfd, (struct sockaddr *) &cli_addr, &length);
        if (socketfd < 0)
            printf("ERROR system call - accept error\n");
        else
        {
            #if OperationMode            
                pthread_t thread_id;

                THREAD_ARGS *args = malloc(sizeof(THREAD_ARGS));

                int * sockAUX = (int *) malloc(sizeof(int *));

                *sockAUX = socketfd;

                args->fd = sockAUX;
                args->hit = hit;

                if (args != NULL) {
                    if (pthread_create(&thread_id, NULL, &attendFTP, args)) {
                        perror("could not create thread");
                        return 1;
                    }
                }

                //pthread_join(thread_id,NULL);
            #else
                pid = fork();
                if(pid==0) {
                    ftp(socketfd, hit);
                } else {
                    //Temos de fechar o socketfd para que seja apenas a child a tratar dos pedidos, caso contrário iria ficar aqui pendurado
                    close(socketfd);
                    kill(pid, SIGCHLD);
                }
            #endif
        }
    }
}

#if OperationMode
    void *attendFTP(void *argp) {
        THREAD_ARGS *args = argp;

        int sock = *args->fd;

        printf("FD SOCK: %d\n\n", sock);

        ftp(sock, args->hit);

        free(args);
        //printf("Thread executou\n\n");
        pthread_exit(NULL);
        return NULL;
    }
#endif

/* this is the ftp server function */
int ftp(int fd, int hit) {
    int j, file_fd, filedesc;
    long i, ret, len;
    char * fstr;
    static char buffer[BUFSIZE + 1]; /* static so zero filled */

    printf("FD: %d\n\n", fd);

    ret = read(fd, buffer, BUFSIZE); // read FTP request

    if (ret == 0 || ret == -1) { /* read failure stop now */
        close(fd);
        return 1;
    }
    if (ret > 0 && ret < BUFSIZE) /* return code is valid chars */
        buffer[ret] = 0; /* terminate the buffer */
    else
        buffer[0] = 0;

    for (i = 0; i < ret; i++) /* remove CF and LF characters */
        if (buffer[i] == '\r' || buffer[i] == '\n')
            buffer[i] = '*';

    printf("LOG request %s - hit %d\n", buffer, hit);

    /* null terminate after the second space to ignore extra stuff */
    for (i = 4; i < BUFSIZE; i++) {
        if (buffer[i] == ' ') { /* string is "GET URL " +lots of other stuff */
            buffer[i] = 0;
            break;
        }
    }

    if (!strncmp(buffer, "get ", 4)) {
        // GET
        getFunction(fd, &buffer[5]);
    } else if (!strncmp(buffer, "ls ", 3)) {
        // LS
        lsFunction(fd,&buffer[3]);
    } else if (!strncmp(buffer, "mget ", 4)) {
        // MGET
        mgetFunction(fd, &buffer[5]);
    }

    sleep(1); /* allow socket to drain before signalling the socket is closed */
    close(fd);
    return 0;
}

void getFunction(int fd, char * fileName){
    int file_fd;
    long ret;

    printf("FD GET: %d\n\n", fd);

    static char buffer[BUFSIZE + 1]; /* static so zero filled */

    if ((file_fd = open(fileName, O_RDONLY)) == -1) { /* open the file for reading */
        printf("ERROR failed to open file %s\n", fileName);
        printf("Err: %d\n\n",errno);
        sprintf(buffer, "%s", "erro");
        write(fd,buffer,BUFSIZE);
        close(fd);
        return;
    }

    printf("GET -> LOG SEND %s \n", fileName);

    /* send file in 8KB block - last block may be smaller */
    while ((ret = read(file_fd, buffer, BUFSIZE)) > 0) {
        write(fd, buffer, ret);
    }
}

void lsFunction(int fd, char * dirName){
    printf("LS -> LOG Header %s \n", dirName);

    static char buffer[BUFSIZE + 1];

    sprintf(buffer, "%s", listFilesDir(dirName));
    write(fd,buffer,BUFSIZE);
}

void mgetFunction(int fd, char *dirName)
{
    FILE *fp;
    char path[255];

    static char buffer[BUFSIZE + 1];

    printf("MGET COUNT -> LOG Header %s \n", dirName);

    sprintf(buffer, "%s", listFilesDir(dirName));
    write(fd,buffer,BUFSIZE);
}

char * listFilesDir(char * dirName)
{
    DIR *midir;
    struct dirent* info_archivo;
    struct stat fileStat;
    char fullpath[256];
    char *files = malloc (sizeof (char) * BUFSIZE);

    if ((midir=opendir(dirName)) == NULL)
    {
        return "\nO directorio pedido não existe.\n";
    }

    while ((info_archivo = readdir(midir)) != 0)
    {
        strcpy (fullpath, dirName);
        strcat (fullpath, "/");
        strcat (fullpath, info_archivo->d_name);
        if (!stat(fullpath, &fileStat))
        {
            if(!S_ISDIR(fileStat.st_mode))
            {
                strcat (files, info_archivo->d_name);
                strcat (files, "$$");
            }
        } else {
            return "\nErro ao ler o directório.\n";
        }
    }
    closedir(midir);

    return files;
}

This is a log example from the server

LOG tftps starting 8181 - pid 15416 FD SOCK: 4

FD: 4

LOG request ls . - hit 1 LS -> LOG Header . FD SOCK: 5

FD: 5

LOG request mget . - hit 2 MGET COUNT -> LOG Header . FD SOCK: 4

FD: 4

FD SOCK: 5

FD: 5

LOG request get /.gitconfig - hit 4 FD GET: 5

GET -> LOG SEND .gitconfig LOG request get /

ERROR failed to open file

FD SOCK: 4

FD: 4

LOG request get /ghostdriver.log - hit 6 FD GET: 4

GET -> LOG SEND ghostdriver.log FD SOCK: 8

FD: 8

LOG request get /.bash_history - hit 7 FD GET: 8

GET -> LOG SEND .bash_history FD SOCK: 6

FD: 6

LOG request get /.profile - hit 5 FD GET: 6

GET -> LOG SEND .profile FD SOCK: 10

FD: 10

LOG request get /.ICEauthority - hit 8 FD GET: 10

GET -> LOG SEND .ICEauthority FD SOCK: 13

FD: 13

FD SOCK: 14

FD SOCK: 22

FD: 22

FD SOCK: 16

FD SOCK: 27

FD: 27

FD SOCK: 18

FD: 18

FD SOCK: 19

LOG request get /.gtk-bookmarks - hit 14 FD SOCK: 25

FD: 25

FD SOCK: 15

LOG request get /.bashrc - hit 20 LOG request get /.bashrc - hit 9 FD GET: 18

FD GET: 25

FD GET: 13

GET -> LOG SEND .bashrc GET -> LOG SEND .bashrc FD SOCK: 23

FD: 23

LOG request get /.zshrc - hit 18 FD SOCK: 26

FD: 26

FD GET: 23

FD SOCK: 29

FD: 29

GET -> LOG SEND .bash_logoutks GET -> LOG SEND .bash_logout FD SOCK: 17

FD: 17

LOG request get /.zsh-update - hit 13 LOG request get /.zsh-update - hit 22 FD SOCK: 28

FD GET: 27

FD: 14

LOG request get /.nano_history - hit 10 LOG request get /.nano_history - hit 24 LOG request get /.nano_history - hit 21 FD: 16

LOG request get /.zsh_history - hit 12 FD SOCK: 21

FD: 21

FD GET: 16

FD: 19

LOG request get /.selected_editor - hit 15 FD SOCK: 24

FD: 24

FD: 15

FD GET: 14

FD GET: 17

FD GET: 29

GET -> LOG SEND .zcompdump-MacPearl-5.0.2 LOG request get /.zcompdump-MacPearl-5.0.2 - hit 17 FD: 28

LOG request get /.Xauthority - hit 23 GET -> LOG SEND .Xauthority GET -> LOG SEND .Xauthority FD GET: 28

LOG request get /.Xauthority - hit 11 GET -> LOG SEND .Xauthority FD GET: 15

GET -> LOG SEND .Xauthority GET -> LOG SEND .Xauthority FD GET: 19

FD GET: 26

GET -> LOG SEND .Xauthority LOG request get /.Xauthority - hit 16 FD GET: 21

GET -> LOG SEND .Xauthority FD GET: 22

LOG request get /.Xauthority - hit 19 GET -> LOG SEND .Xauthority GET -> LOG SEND .Xauthority GET -> LOG SEND .Xauthority FD GET: 24

GET -> LOG SEND .Xauthority FD SOCK: 30

FD: 30

FD SOCK: 31

FD: 31

LOG request get /.zcompdumpy - hit 25 LOG request get /.zcompdump - hit 26 FD GET: 30

FD GET: 31

GET -> LOG SEND .zcompdump GET -> LOG SEND .zcompdump

I can see that sometimes he's using the same filedescriptor in more than one thread. I just can't figure out how I can solve this sync problem.

GShaik
  • 197
  • 1
  • 17
rafaelcpalmeida
  • 874
  • 1
  • 9
  • 28
  • What is this: 'static char buffer[BUFSIZE + 1];' ? Please explain 'static' in multithreaded code:(( – Martin James Apr 07 '16 at 09:43
  • Put another way, for multithreaded functions, 'static' is a kiss of death. – Martin James Apr 07 '16 at 09:46
  • The base code was given and we're meant to improve the code from a monoprocessing to multiprocess and multithreading. That part of static was already made – rafaelcpalmeida Apr 07 '16 at 09:47
  • Get rid of it! Nasty, evil static! Check over the rest of the code carefully - anyone daft enough to use static in a library has probably screwed up more stuff:( – Martin James Apr 07 '16 at 09:57
  • Removing static is enough? – rafaelcpalmeida Apr 07 '16 at 09:58
  • No idea - I didn't write it. A quick look shows that many of the other functions have static trash too:( You will have to analyse the functions, find out why the buffers were made static in the first place and then get rid of them in an appropriate manner. Just replacing them with auto storage may work, or it may not.. – Martin James Apr 07 '16 at 10:02
  • Never mind - I just looked over the rest of the FTP lib code. My best advice - drop it. It has too many serious problems and,having first seen the 'static', I'm not hugely surprised:( – Martin James Apr 07 '16 at 10:10
  • It is not safe to use even with one thread. – Martin James Apr 07 '16 at 10:11
  • @MartinJames can you, please, explain-me why is it so bad to use 'static'? This code was given by my teachers and when I remove the 'static' keyword it appears to work correctly – rafaelcpalmeida Apr 07 '16 at 10:33

1 Answers1

1

Get rid of the 'static' storage qualifier on declarations such as:

static char buffer[BUFSIZE + 1];

You must not use freely the static qualifier on data that is subject to access from multiple threads - there is only one instance of such data globally and, unless extra explicit synchronization is applied, the threads will chop and slice the data to pieces.

If you use automatic storage, all common C compilers place data on the current stack. With multiple threads, ie. multiple stacks, that means one data item per thread and so no slicing and dicing.

Note that the static buffers in your code have a comment: '/* static so zero filled */'. There may be implications for the rest of the code/data of removing the static - automatic vars are NOT initialized to zero, and any code that relies on such will be compromised.

Note also that the code has other issues, unrelated to multithreading. The 'ftp' function fails to correctly handle the octet stream nature of TCP and assumes, wrongly, that messages larger than one byte will be received in their entirety with one call to read() in default byte-stream mode.

This code sucks, even for non-library code, and the author should be fired:)

rafaelcpalmeida
  • 874
  • 1
  • 9
  • 28
Martin James
  • 24,453
  • 3
  • 36
  • 60
  • I went from `static char buffer[BUFSIZE + 1];` to `char buffer[BUFSIZE + 1] = {0};` and everything appears to work correctly now. I can't just get rid of this code as this is for a University project and the main code was written by our teachers :-) – rafaelcpalmeida Apr 07 '16 at 11:20
  • 'main code was written by our teachers ' - lol yes, sounds about right :) – Martin James Apr 07 '16 at 11:22