HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

TCP Tunnel in C

Submitted by: @import:stackexchange-codereview··
0
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

Solution

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:

  • 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 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;
    }

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.