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.
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:)