7
\$\begingroup\$

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;
}
\$\endgroup\$
7
\$\begingroup\$

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;
    }
\$\endgroup\$
  • \$\begingroup\$ Makes a lot of sense, thank you. I have a question about the double close though, since either thread could be the one to choose to close the connections first, what is the most effective way to close the sockets? \$\endgroup\$ – JavaProphet Jul 26 '15 at 9:40
  • \$\begingroup\$ @JavaProphet Probably you should pass the thread id of thread #1 to thread #2, and then have thread #2 call pthread_join(tid1, NULL) and then close both fds. That way you know both threads are finished using the fds before you close them. \$\endgroup\$ – JS1 Jul 26 '15 at 9:49
  • \$\begingroup\$ Alright, thanks. Would it be appropriate to modify the OP with any refactored code, or no? \$\endgroup\$ – JavaProphet Jul 26 '15 at 9:51
  • \$\begingroup\$ @JavaProphet No don't do that. You should wait for other reviews. Then later, you can post a followup question with updated code if you want another review. \$\endgroup\$ – JS1 Jul 26 '15 at 9:54
  • \$\begingroup\$ Okay. I'm writing in that pthread_join stuff, and I realized that it could just end up blocking on the other thread's read/write, so I ended up passing a pointer to both, so whichever one gets to the ret: label will cancel the other thread and then close. Thoughts? \$\endgroup\$ – JavaProphet Jul 26 '15 at 10:01
2
\$\begingroup\$
  • 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.
\$\endgroup\$
  • \$\begingroup\$ Thank you for all the information, much appreciated. I do have one question about the goto, in this case it is a good idea to replace with break, but in a multi-layer loop, I read that goto is the only way out. I also found it's a good way to manage memory paired with the "only one return" idea, when you have one near the end to free resources if not NULL. What do you think of this? \$\endgroup\$ – JavaProphet Jul 28 '15 at 2:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.