TCP Tunnel in C

Question

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 <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <pthread.h>
#include <errno.h>
#include <signal.h>

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 < 1) {
            goto ret;
        }
        int w = 0;
        while (w < i) {
            w += write(fcll, buf + w, i - w);
        }
    }
    ret: close(scll);
    close(fcll);
    free(buf);
}
void ftc(void *arg) {
    int scll = scl;
    int fcll = fcl;
    void* buf = malloc(1024);
    while (1) {
        int i = read(fcll, buf, 1024);
        if (i < 1) {
            goto ret;
        }
        int w = 0;
        while (w < i) {
            w += write(scll, buf + w, i - w);
        }
    }
    ret: close(scll);
    close(fcll);
    free(buf);
}
void lcl(int signum) {
    close(ss);
}
int main(int argc, char** args) {
    if (argc != 5) {
        printf("Usage: TCPTunnel <bind-ip> <bind-port> <forward-ip> <forward-port>\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 < 0) {
        printf("Failed to create socket, errno=%i\n", errno);
        return 1;
    }
    struct sockaddr_in sa;
    memset(&sa, 0, sizeof(sa));
    if (inet_aton(args[1], &sa.sin_addr.s_addr) == 0) {
        printf("Invalid bind IP!");
        return 1;
    }
    sa.sin_family = AF_INET;
    sa.sin_port = htons(strtol(args[2], NULL, 0));
    struct sockaddr_in fa;
    socklen_t fal;
    memset(&fa, 0, sizeof(fa));
    if (inet_aton(args[3], &fa.sin_addr.s_addr) == 0) {
        printf("Invalid forward IP!");
        return 1;
    }
    fa.sin_family = AF_INET;
    fa.sin_port = htons(strtol(args[4], NULL, 0));
    fal = sizeof(fa);
    int e = bind(ss, (struct sockaddr *) &sa, sizeof(sa));
    if (e < 0) {
        printf("Failed to bind socket, errno=%i\n", errno);
        return 1;
    }
    e = listen(ss, 50);
    if (e < 0) {
        printf("Failed to listen to socket, errno=%i\n", errno);
        return 1;
    }
    struct sockaddr_in ca;
    socklen_t cal;
    while (1) {
        int cs = accept(ss, (struct sockaddr *) &ca, &cal);
        if (cs < 0) {
            break;
        }
        scl = cs;
        fcl = socket(AF_INET, SOCK_STREAM, 0);
        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;
        }
        pthread_t ptt, ptt2;
        pthread_create(&ptt, NULL, ctf, NULL);
        pthread_create(&ptt2, NULL, ftc, NULL);
    }
    return 0;
}

Answer 1

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

Answer 2

• Comments: comments are useful, especially if the program should be reviewed: e.g. a short description of the functions.
• Names: meaningful names for function and variables would be useful , too.

• System Calls: For the system calls like malloc or write you should check if it was successful. The simplest way of process an error is to write an error message and exit the program. That is better than ignoring errors and continuing processing as you do. Maybe system calls that return resources like close and free must not be checked. You can use macros that return the filename and the line number to include filename and line number inthese error messages

• Error Messages:

• stderr: In Unix/Linux error messages should be to stderr and not to stdin. stdin is intended for data.
• System Error Message: the function char *strerror(int errnum); can be used to get the error message. return this error text to the user

• Return Codes don't return always 1 in case of an error. If you have an error resulting from a system call, return it's error number. If you have other errors then return different error numbers for different errors.

• goto: the goto should be avoided. You can use break to exit a loop. But in C the following is prefered:

while ((i = read(scll, buf, 1024)<1){
  ...
}

• lint: Check the warnings created by the compiler. You can use lint to check your code. Here is a version accessible over the web.