Return to Answer

##Cryptic variable names##

I had a hard time reading your code due to all of the short variable names such as cs, ss, scl, cal, ca, etc. It would be very helpful if you used descriptive names.

##Unsafe argument passing to threads##

Right now, your threads require two arguments, the file descriptors scl and fcl. The threads are getting these arguments from global variables. This is unsafe because your main loop proceeds to accept new connections, which means the loop in main could possibly change the globals before the threads have read them into local variables. It would be best to pass your arguments through malloc'd structs instead.

##Double close()##

Each of your two threads closes scll and fcll before exiting. That means you are closing each fd twice when only one close() is needed. This could cause a problem in the following sequence:

1. Thread one closes scll (let's call it file descriptor 5).
2. The main loop accepts a new connection, using file descriptor 5, since it was not in use.
3. Thread two closes scll which closes the fd for the new connection.

##goto ret = break##

Where you have goto ret, you could have written break.

##Missing error checks##

You check for read() returning -1, but not for write() returning -1. Also, you are not checking the return values of malloc() and pthread_create(), which could conceivably fail if you create too many threads.

##Combine two if statements##

These two if statements:

 if (fcl < 0) {
 printf("Failed to connect to forwarding server for client!\n");
 close(cs);
 close(fcl);
 continue;
 }
 if (connect(fcl, (struct sockaddr *) &fa, fal) < 0) {
 printf("Failed to connect to forwarding server for client!\n");
 close(cs);
 close(fcl);
 continue;
 }

could be merged to:

 if (fcl < 0 || connect(fcl, (struct sockaddr *) &fa, fal) < 0) {
 printf("Failed to connect to forwarding server for client!\n");
 close(cs);
 close(fcl);
 continue;
 }

Cryptic variable names

I had a hard time reading your code due to all of the short variable names such as cs, ss, scl, cal, ca, etc. It would be very helpful if you used descriptive names.

Unsafe argument passing to threads

Right now, your threads require two arguments, the file descriptors scl and fcl. The threads are getting these arguments from global variables. This is unsafe because your main loop proceeds to accept new connections, which means the loop in main could possibly change the globals before the threads have read them into local variables. It would be best to pass your arguments through malloc'd structs instead.

Double close()

Each of your two threads closes scll and fcll before exiting. That means you are closing each fd twice when only one close() is needed. This could cause a problem in the following sequence:

1. Thread one closes scll (let's call it file descriptor 5).
2. The main loop accepts a new connection, using file descriptor 5, since it was not in use.
3. Thread two closes scll which closes the fd for the new connection.

goto ret = break

Where you have goto ret, you could have written break.

Missing error checks

You check for read() returning -1, but not for write() returning -1. Also, you are not checking the return values of malloc() and pthread_create(), which could conceivably fail if you create too many threads.

Combine two if statements

These two if statements:

 if (fcl < 0) {
 printf("Failed to connect to forwarding server for client!\n");
 close(cs);
 close(fcl);
 continue;
 }
 if (connect(fcl, (struct sockaddr *) &fa, fal) < 0) {
 printf("Failed to connect to forwarding server for client!\n");
 close(cs);
 close(fcl);
 continue;
 }

could be merged to:

 if (fcl < 0 || connect(fcl, (struct sockaddr *) &fa, fal) < 0) {
 printf("Failed to connect to forwarding server for client!\n");
 close(cs);
 close(fcl);
 continue;
 }