Network Interface Object

Question

I wrote a Network... thing (not really telnet); and it's pretty simple. No options, just straight I/O. It seems programs get so bloated easily.

#include <arpa/inet.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 non-block 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) {
 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);
 else 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 = 0;
 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);
 memset(host, 0, 64);
 memcpy(host, addr_tmp, strlen(addr_tmp));
 freeaddrinfo(servinfo);
 return rv;
}
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 > 0? 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;
}

• user20300: FWIW, some nitpicks: 1. using FULL_CAPS for C constant values (most people include enums here) seems to be the norm for readability, 2. prefer e_socket_msg* to e_socket_msg[] for string constants (i.e. unless you explicitly want to edit that string later on), 3. since your errors are 1:1 mapped to error codes, use a map (or even simply an array, when your key values are consecutive integers) and access it to get messages, instead of switching over the key.
• slebetman: You have not implemented telnet. Telnet is an actual protocol, not just a raw TCP session. You need to handle the byte 0xff as a special case. To send 0xff you need to send 0xff,0xff. You also need to handle \r as a special case - it must be followed by 0x00. Note that \r is different depending on weather you are talking to an EBDIC or ASCII system but I think we can safely ignore EBDIC these days unless we're talking to IBM mainframes. So assuming ASCII, every time you detect 0x0d that is not followed by 0x0a it must be sent as 0x0d, 0x00. But if it is followed by 0x0a it is 0x0d, 0x0a
• slebetman: What you have implemented here is something similar to netcat
• sudo rm -rf slash: A lot of the code seems to be based on error handling. Look into errno and strerror... Let the c libraries handle this all for you. It's more elegant, smaller, and won't be wrong

Answer 1

• There is no guarantee that write writes out the entire buffer. This means that

  if (FD_ISSET(STDIN_FILENO, &fds))
   rv = transfer(STDIN_FILENO, buffer, BUFLEN, sd);
  

  may lose data. Consider looping until all data gone out.

• As long as the portability is concerned, the timeout parameter of select needs attention. The BSD implementation doesn't change it, Linux does (POSIX considers both compliant), WSA says nothing. It is prudent to re-initialize tv inside the loop.

• Getting host and port from stdin seriously restricts the utility of the program. It cannot be used as a filter. Consider passing them via command line arguments.

• Make up your mind on who is responsible for error reporting. For example, resolve does call print_error, and so does main on resolve's failure. The error message gots printed twice.

• geraddrinfo returns some valuable information in servinfo, which you effectively throw away - what was the reason to call it?. Still you should freeaddrinfo afterwards.

Answer 2

the posted code causes the compiler to output several warning messages.

When compiling, always enable the warnings, then fix those warnings.

Here is the compile statement and the resulting warning messages.

gcc -Wall -Wextra -Wconversion -pedantic -std=gnu11 -ggdb -c "untitled2.c" 
untitled2.c: In function ‘input’:
untitled2.c:86:37: warning: conversion to ‘size_t {aka long unsigned int}’ from ‘int’ may change the sign of the result [-Wsign-conversion]
 rv = read(STDIN_FILENO, output, len - 1);
 ^~~
untitled2.c:86:10: warning: conversion to ‘int’ from ‘ssize_t {aka long int}’ may alter its value [-Wconversion]
 rv = read(STDIN_FILENO, output, len - 1);
 ^~~~
untitled2.c: In function ‘sig_handler’:
untitled2.c:90:22: warning: unused parameter ‘sig’ [-Wunused-parameter]
 void sig_handler(int sig) {
 ^~~
untitled2.c: In function ‘connect_to’:
untitled2.c:127:27: warning: conversion to ‘uint16_t {aka short unsigned int}’ from ‘int’ may alter its value [-Wconversion]
 addr.sin_port = htons(port);
 ^~~~
untitled2.c: In function ‘transfer’:
untitled2.c:138:32: warning: conversion to ‘size_t {aka long unsigned int}’ from ‘int’ may change the sign of the result [-Wsign-conversion]
 int len = read(fd_in, buf, buf_len);
 ^~~~~~~
untitled2.c:138:15: warning: conversion to ‘int’ from ‘ssize_t {aka long int}’ may alter its value [-Wconversion]
 int len = read(fd_in, buf, buf_len);
 ^~~~
untitled2.c:139:46: warning: conversion to ‘size_t {aka long unsigned int}’ from ‘int’ may change the sign of the result [-Wsign-conversion]
 return len > 0? len - write(fd_out, buf, len) : -1;
 ^~~
untitled2.c:139:51: warning: conversion to ‘int’ from ‘ssize_t {aka long int}’ may alter its value [-Wconversion]
 return len > 0? len - write(fd_out, buf, len) : -1;