patterncMinor
TCP Tunnel in C
Viewed 0 times
tcptunnelstackoverflow
Problem
I'm completely aware that I'm not using NIO, but this is my first polished C project. All criticism is welcome, if I'm doing something wrong, I want to nip it in the bud before I'm used to it.
`/*
* TCPTunnel.c
*
* Created on: Jul 25, 2015
* Author: javaprophet
*/
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
int scl = -1;
int fcl = -1;
int ss = -1;
void ctf(void *arg) {
int scll = scl;
int fcll = fcl;
void* buf = malloc(1024);
while (1) {
int i = read(scll, buf, 1024);
if (i \n");
return 1;
}
struct sigaction action;
memset(&action, 0, sizeof(struct sigaction));
action.sa_handler = lcl;
sigaction(SIGTERM, &action, NULL);
ss = socket(AF_INET, SOCK_STREAM, 0);
if (ss
`/*
* TCPTunnel.c
*
* Created on: Jul 25, 2015
* Author: javaprophet
*/
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
int scl = -1;
int fcl = -1;
int ss = -1;
void ctf(void *arg) {
int scll = scl;
int fcll = fcl;
void* buf = malloc(1024);
while (1) {
int i = read(scll, buf, 1024);
if (i \n");
return 1;
}
struct sigaction action;
memset(&action, 0, sizeof(struct sigaction));
action.sa_handler = lcl;
sigaction(SIGTERM, &action, NULL);
ss = socket(AF_INET, SOCK_STREAM, 0);
if (ss
Solution
Cryptic variable names
I had a hard time reading your code due to all of the short variable names such as
Unsafe argument passing to threads
Right now, your threads require two arguments, the file descriptors
Double close()
Each of your two threads closes
goto ret = break
Where you have
Missing error checks
You check for
Combine two if statements
These two if statements:
could be merged to:
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:- Thread one closes
scll(let's call it file descriptor 5).
- The main loop accepts a new connection, using file descriptor 5, since it was not in use.
- Thread two closes
scllwhich 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;
}Code Snippets
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;
}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;
}Context
StackExchange Code Review Q#98100, answer score: 7
Revisions (0)
No revisions yet.