current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 11 down vote favorite

After studying and considering the reviews for this code, I began working on the code, and came up with this. It has much better error and disconnect detection, and it also resolves addresses:

#include <arpa/inet.h>
#include <error.h>
#include <fcntl.h>
#include <netdb.h>
#include <signal.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <unistd.h>

#define BUFLEN  1024

sig_atomic_t run = 1;
sig_atomic_t sd = 1;

char e_socket_msg[]     = "socket creation failed\n";
char e_sockopt_msg[]    = "set socket nonblock failed\n";
char e_parse_msg[]      = "address parsing failed\n";
char e_timeout_msg[]    = "connection attempt timed out\n";
char e_io_msg[]         = "i/o error\n";
char e_generic_msg[]    = "unknown or unexpected error\n";
char e_resolve_msg[]    = "unable to resolve address\n";

typedef enum {
    e_resolve   = -1,
    e_socket    = -2,
    e_sockopt   = -3,
    e_parse     = -4,
    e_timeout   = -5,
    e_io        = -6
} Error;

void input(char *input, char *output, int len);
int resolve(char *host);
void sig_handler(int sig);
int connect_to(char *host, int port);
int transfer(int fd_in, char *buf, int buf_len, int fd_out);
int print_error(Error e);

int main(void) {
    char ch;
    fd_set fds;
    struct timeval tv;
    int rv;
    char buffer[BUFLEN];
    char host[64], port[16];
    char host_msg[] = "host:\t";
    char port_msg[] = "port:\t";

    input(host_msg, host, 64);
    input(port_msg, port, 16);

    sd = connect_to(host, atoi(port));
    if (sd < 0) {
        rv = resolve(host);
        if (rv < 0) return print_error(rv);
        sd = connect_to(host, atoi(port));
        if (sd < 0) return print_error(sd);
    }

    signal(SIGINT, sig_handler);
    signal(SIGPIPE, sig_handler);

    FD_ZERO(&fds);
    tv.tv_sec = 0;
    tv.tv_usec = 300000;
    while (run) {
        FD_SET(sd, &fds);
        FD_SET(STDIN_FILENO, &fds);
        rv = select(sd + 1, &fds, NULL, NULL, &tv);
        if (FD_ISSET(STDIN_FILENO, &fds))
            rv = transfer(STDIN_FILENO, buffer, BUFLEN, sd);
        if (FD_ISSET(sd, &fds))
            rv = transfer(sd, buffer, BUFLEN, STDOUT_FILENO);
        if (rv != 0) {
            run = 0;
            if (rv > 0) print_error(e_io);
        }
    }
    close(sd);
    return 0;
}

void input(char *input, char *output, int len) {
    int rv;

    (void) write(STDOUT_FILENO, input, strlen(input));
    rv = read(STDIN_FILENO, output, len - 1);
    output[rv - 1] = '\0';
}

void sig_handler(int sig) {
    run = 0;
    close(sd);
}

int resolve(char *host) {
    struct addrinfo hints, *servinfo;
    struct in_addr addr;
    char *addr_tmp;
    int rv;

    memset(&hints, 0, sizeof hints);
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_family = AF_INET;
    rv = getaddrinfo(host, NULL, &hints, &servinfo);
    if (rv) return print_error(e_resolve);
    addr.s_addr = ((struct sockaddr_in*)servinfo->ai_addr)->sin_addr.s_addr;
    addr_tmp = inet_ntoa(addr);
    memcpy(host, addr_tmp, strlen(addr_tmp));
    freeaddrinfo(servinfo);
}

int connect_to(char *host, int port) {
    int sd;
    struct sockaddr_in addr;
    fd_set sfds;
    struct timeval tv;

    sd = socket(AF_INET, SOCK_STREAM, 0);
    if (sd == -1) return e_socket;
    if (fcntl(sd, F_SETFL, O_NONBLOCK) == -1) return e_sockopt;
    memset(&addr, 0, sizeof (addr));
    addr.sin_family = AF_INET;
    if (inet_pton(AF_INET, host, &addr.sin_addr) != 1)
        return e_parse;
    addr.sin_port = htons(port);
    connect(sd, (struct sockaddr *) &addr, sizeof (addr));
    FD_ZERO(&sfds);
    FD_SET(sd, &sfds);
    tv.tv_sec = 4;
    tv.tv_usec = 0;
    if (select(sd + 1, NULL, &sfds, NULL, &tv)) return sd;
    return e_timeout;
}

int transfer(int fd_in, char *buf, int buf_len, int fd_out) {
    int len = read(fd_in, buf, buf_len);
    return len ? len - write(fd_out, buf, len) : -1;
}

int print_error(Error e) {
    char *msg;

    switch (e) {
        case e_socket:
            msg = e_socket_msg;
            break;
        case e_sockopt:
            msg = e_sockopt_msg;
            break;
        case e_parse:
            msg = e_parse_msg;
            break;
        case e_timeout:
            msg = e_timeout_msg;
            break;
        case e_io:
            msg = e_io_msg;
            break;
        case e_resolve:
            msg = e_resolve_msg;
            break;
        default:
            msg = e_generic_msg;
            break;
    }
    (void) write(STDERR_FILENO, msg, strlen(msg));
    return -e;
}

I know it's more of a read than my last post, but it should be pretty straight forward. It creates an interactive TCP connection. I modified the code a bit to be usable with "c4droid" through the Android software store. I should note with some rooted phones, this program may possibly not behave properly; as some implementations of root access with Android employ a workaround involving constantly flushing standard streams.

share|improve this question

2 Answers 2

up vote 6 down vote accepted

Return value handling

There are a few places with potential problems involving return values:

resolve()

    rv = resolve(host);
    if (rv < 0) return print_error(rv);

This seems ok until you look at resolve():

int resolve(char *host) {
    struct addrinfo hints, *servinfo;
    struct in_addr addr;
    char *addr_tmp;
    int rv;  

    memset(&hints, 0, sizeof hints);
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_family = AF_INET;
    rv = getaddrinfo(host, NULL, &hints, &servinfo);
    if (rv) return print_error(e_resolve);
    addr.s_addr = ((struct sockaddr_in*)servinfo->ai_addr)->sin_addr.s_addr;
    addr_tmp = inet_ntoa(addr);
    memcpy(host, addr_tmp, strlen(addr_tmp));
    freeaddrinfo(servinfo);
}
  1. There's no return value at the end of the function. You probably meant return 0 at the end.
  2. If you look at what return print_error(e_resolve) actually does, it returns 1. This is bad because it is a positive value and the caller apparently expects a negative value to mean an error. You can change this to return e_resolve to fix the problem, as the caller will print the error already.

Main loop

while (run) {
    FD_SET(sd, &fds);
    FD_SET(STDIN_FILENO, &fds);
    rv = select(sd + 1, &fds, NULL, NULL, &tv);
    if (FD_ISSET(STDIN_FILENO, &fds))
        rv = transfer(STDIN_FILENO, buffer, BUFLEN, sd);
    if (FD_ISSET(sd, &fds))
        rv = transfer(sd, buffer, BUFLEN, STDOUT_FILENO);
    if (rv != 0) {
        run = 0;
        if (rv > 0) print_error(e_io);
    }
}

Here, I don't like how you don't check the return value of select(). You set rv to the return value, but you never check it. If select() actually returns an error, the contents of fds becomes undefined. This could cause your code to possibly proceed with a transfer, wiping out the value of rv and losing the fact that select() returned an error.

The same thing applies to the return value of the first transfer() call. You never check the return value, so it may proceed to the second transfer and wipe out the contents of rv, losing the error from the first transfer.

transfer()

int transfer(int fd_in, char *buf, int buf_len, int fd_out) {
    int len = read(fd_in, buf, buf_len);
    return len ? len - write(fd_out, buf, len) : -1;
}

First of all, this code is kind of confusing. I think it is trying to return -1 for a read failure, 0 for success, and a positive number for a write failure. There should be a comment describing this, because it isn't obvious. But there is a problem. What exactly happens if read returns an error (i.e. -1)?

  1. len = -1
  2. return -1 - write(fd_out, buf, -1)
  3. Since the 3rd argument is unsigned: write(fd_out, buf, 0xffffffff)
  4. Most likely: segmentation violation
share|improve this answer
up vote 7 down vote

It looks good, though some parts are too tightly packed for my taste. This is certainly subjective, but I would like to have some more blank lines in a few places, to better separate the code into distinct "paragraphs".

Compiler warnings:

Building your source file with: clang -std=c89 -Wall -Wextra -pedantic test.c produced a couple warnings:

test.c:57:10: warning: unused variable 'ch' [-Wunused-variable]
       char ch;
             ^
test.c:108:22: warning: unused parameter 'sig' [-Wunused-parameter]
void sig_handler(int sig) {
                     ^
test.c:128:1: warning: control may reach end of non-void function [-Wreturn-type]
}
^
3 warnings generated.

The first two are harmless unused variables, nevertheless, should be fixed. The third one is very serious and renders your code undefined behavior. resolve() should return an integer value, which is expected in its call site:

rv = resolve(host);

But it is not returning on all paths.

This is just one reason why compiling with warnings enabled is very important! Another thing that you might consider doing every once in a while is running a static code analyser on your project.

Use const where appropriate:

Your program doesn't need to edit these error messages during runtime. Make them const to state that clearly to the reader:

const char e_socket_msg[]  = "socket creation failed\n";
const char e_sockopt_msg[] = "set socket nonblock failed\n";
const char e_parse_msg[]   = "address parsing failed\n";
const char e_timeout_msg[] = "connection attempt timed out\n";
const char e_io_msg[]      = "i/o error\n";
const char e_generic_msg[] = "unknown or unexpected error\n";
const char e_resolve_msg[] = "unable to resolve address\n";

Note: This will also require changing the pointer in print_error() to be a const char*.

const should also be applied to pointer parameters of functions, when the function is only meant to read from that pointer. If you observe the C library, you will notice that a function like printf, for instance, takes a const char*, because it only prints the string, without changing it.

Avoid function prototypes where practical:

You could have avoided the function prototypes in this case by placing all the other functions before main(). The nuisance with prototypes is that they introduce duplication. When you make a change you have at least two places to update.

Choose good names for variables with big scope:

These two globals:

sig_atomic_t run = 1;
sig_atomic_t sd = 1;

Have very vagues names for their scope. The larger the scope of a variable, the better and more descriptive its name should be. In C, you also have to take care to avoid name collision, since everything resided inside the global namespace.

Consider always using { }:

This is more of a personal preference, and certainly arguable, but I find it more consistent to always supply a pair of curly braces, even for the single line statements.

One could also claim that it makes it easier to maintain and extend the code and it might help detecting mistakes like the goto fail SSL bug.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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