A simple server-client application in C

Question

I am reading "The Linux Programming Interface" by Michael Kerrisk. I'm studying about sockets, and I made a simple application using sockets on the unix domain.

I want to know if I'm using sockets properly. Also what features should I add?

There are two programs: server and client. client requires an input file, and it sends the contents of the file to server. server receives the data and prints it out.

Code:

1. server.c

#include <sys/socket.h>
#include <sys/un.h>
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#define BUFFER_LEN 128
void fatal(char* message) {
 // Print error message and exit
 perror(message);
 exit(EXIT_FAILURE);
}
void socket_comm() {
 const char* SOCKET_PATH = "/home/tux/temp/server_sock";
 int socket_fd;
 // Create socket
 socket_fd = socket(AF_UNIX, SOCK_STREAM, 0);
 if (socket_fd == -1) {
 fatal("Error while initializing socket");
 }
 // Bind socket to SOCKET_PATH
 socklen_t socket_addr_size;
 struct sockaddr_un socket_addr;
 memset(&socket_addr, 0, sizeof(struct sockaddr_un)); // Initialize to 0
 socket_addr.sun_family = AF_UNIX;
 strncpy(socket_addr.sun_path, SOCKET_PATH, sizeof(socket_addr.sun_path) - 1);
 socket_addr_size = sizeof(socket_addr);
 if (bind(socket_fd, (struct sockaddr *) &socket_addr, sizeof(struct sockaddr_un)) == -1) {
 fatal("Error while binding socket");
 } else {
 printf("Successfully generated server-side socket\n");
 }
 // Mark as passive
 if (listen(socket_fd, 8) == -1) {
 fatal("Cannot mark socket as passive");
 } 
 
 // Connect to client and receive data
 ssize_t numread;
 char buffer[BUFFER_LEN + 1];
 while (1) {
 // Connect to client
 int connect_fd = accept(socket_fd, (struct sockaddr*) &socket_addr, &socket_addr_size);
 if (connect_fd == -1) {
 fatal("Cannot accept connections");
 sleep(1);
 }
 
 printf("Connected to client\n");
 // Receive data
 while ((numread = read(connect_fd, buffer, BUFFER_LEN)) > 0) {
 buffer[numread] = '0円';
 printf("%s", buffer); 
 }
 }
}
int main() {
 socket_comm();
 return 0;
}

2. client.c

#include <sys/socket.h>
#include <sys/un.h>
#include <string.h>
#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <unistd.h>
void fatal(char* message) { 
 // Print message and exit
 perror(message);
 exit(EXIT_FAILURE);
}
long get_data(int argc, char* argv[], char** buffer) {
 // Command line argument must be path to file
 // option "-h" or "--help" prints help
 if (argc != 2 || strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-h") == 0) {
 printf("Usage: %s filepath\n",argv[0]);
 printf("Sends data to the server\n");
 exit(EXIT_SUCCESS);
 }
 // Open file
 FILE* fp;
 fp = fopen(argv[1],"r");
 if (fp == NULL) {
 fatal("Cannot open file");
 }
 // Determine file size
 if (fseek(fp, 0, SEEK_END) == -1) {
 fatal("Error determining file size");
 }
 long size = ftell(fp);
 if (size == -1) {
 fatal("Error determining file size");
 }
 rewind(fp);
 
 // Allocate memory for buffer
 *buffer = malloc(size + 1);
 if (*buffer == NULL) {
 fatal("Error allocating memory");
 }
 memset(*buffer, 0, size + 1);
 // Read file and store data in buffer
 fread(*buffer, 1, size, fp);
 if (ferror(fp) != 0) {
 fatal("Error reading file");
 }
 fclose(fp);
 return size;
}
void socket_comm(char* buffer, long size) {
 const char* PEER_SOCKET_PATH = "/home/tux/temp/server_sock";
 int socket_fd;
 // Create socket
 socket_fd = socket(AF_UNIX, SOCK_STREAM, 0);
 if (socket_fd == -1) {
 fatal(strerror(errno));
 }
 
 // Connect to server
 struct sockaddr_un peer_socket_addr;
 memset(&peer_socket_addr, 0, sizeof(struct sockaddr_un));
 peer_socket_addr.sun_family = AF_UNIX;
 strncpy(peer_socket_addr.sun_path, PEER_SOCKET_PATH, sizeof(peer_socket_addr.sun_path) - 1);
 
 if (connect(socket_fd, (struct sockaddr*) &peer_socket_addr, sizeof(peer_socket_addr)) == -1) {
 fatal("Error while connecting to server");
 }
 
 printf("Connected to server.\n");
 // Send data to server
 if (write(socket_fd, buffer, size) == -1) {
 fatal("Error sending data to server");
 }
 return;
}
int main(int argc, char* argv[]) {
 char* file_buffer;
 long file_size;
 
 // Get data and file size
 file_size = get_data(argc, argv, &file_buffer);
 // Send data to server
 socket_comm(file_buffer, file_size);
 // Free memory
 free(file_buffer);
 return 0;
}

Codes can be downloaded in my github repo.

• guest271314: This bruinsslot.jp/post/simple-http-webserver-in-c was helpful to me github.com/jpbruinsslot/webserver-c

Answer 1

Reinderien's great answer focused on the readability and maintainability. I will focus on your other question:

Also what features should I add?

Before I answer this question, let's take a step back and think about the current code. It feels a bit lopsided, doesn't it? On one hand, we have sockets, and thus use write and read with file descriptors (int), on the other side, we have stdout (via printf) and a FILE*. This feels like a mixture of different APIs, and it is. This mixture of APIs leads to small mistakes, because we expect one API (write) to act the same as the other (printf), yet it doesn't:

 // Send data to server
 if (write(socket_fd, buffer, size) == -1) {
 fatal("Error sending data to server");
 }

This may or may not send the full file to the server. write is allowed to send less than size, yet we never checked that.

With that in mind, I suggest two one major change: use plain file descriptors for the non-socket part, too, that is: use void socket_comm(int fd), for both the server as well as the client:

• in the client, run r = read(file_fd, buf, count) and then write(socket, buf, r) in a loop
• in the server, run r = read(socket, buf, count) and then write(STDOUT_FILENO, buf, r) in a loop

You can immediately see the symmetry in both the server and client there; the inner loop becomes the same. Also, the client doesn't have to keep the whole file in memory, which can be a "small" issue on large files.

Note that this also allows you later on to use functions like sendfile or splice, which allow you to copy the data within the kernel instead of using a user-space buffer. Note that the same holds for those functions as for read and write: you have to inspect the result to see how many bytes have been actually written and read.

int get_input_fd(int argc, char* argv[]) {
 // Command line argument must be path to file
 // option "-h" or "--help" prints help
 if (argc != 2 || strcmp(argv[1], "--help") == 0 ||
 // exercise: do not return EXIT_SUCCESS on wrong usage
 // exercise: if no file is given, send standard input to server,
 // i.e. make it possible to write text to the server on
 // your terminal, or make it possible to stream into the
 // client, e.g. `./client < input_file` or `grep pattern file | ./client`
 strcmp(argv[1], "-h") == 0) {
 printf("Usage: %s filepath\n", argv[0]);
 printf("Sends data to the server\n");
 exit(EXIT_SUCCESS);
 }
 int fd = open(argv[1], O_RDONLY);
 if (fd < 0) {
 fatal("Cannot open file");
 }
 return fd;
}
void socket_comm(int input) {
 const char* PEER_SOCKET_PATH = "/home/tux/temp/server_sock";
 const int socket_fd = socket(AF_UNIX, SOCK_STREAM, 0);
 if (socket_fd == -1) {
 fatal(strerror(errno));
 }
 // Connect to server
 struct sockaddr_un peer_socket_addr;
 memset(&peer_socket_addr, 0, sizeof(peer_socket_addr));
 peer_socket_addr.sun_family = AF_UNIX;
 strncpy(peer_socket_addr.sun_path, PEER_SOCKET_PATH,
 sizeof(peer_socket_addr.sun_path) - 1);
 if (connect(socket_fd, (struct sockaddr*)&peer_socket_addr, sizeof(peer_socket_addr)) == -1) {
 fatal("Error while connecting to server");
 }
 printf("Connected to server.\n");
 ssize_t sendfile_ret = sendfile(input, socket_fd, NULL, 2048);
 if (sendfile_ret >= 0) {
 // input is a file, use sendfile as optimization
 while ((sendfile_ret = sendfile(input, socket_fd, NULL, 2048)) > 0) {
 // sendfile is handling all the work
 }
 // exercise: check whether an error occured
 } else {
 // exercise: fallback to read(2) + write(2) if input is not a file
 }
}
int main(int argc, char* argv[]) {
 int fd = get_input_fd(argc, argv);
 // Send data to server
 socket_comm(fd);
 close(fd);
 return 0;
}

I've included four exercises for additional features:

• the fd example uses sendfile, which would fail if input isn't backed by an actual file. Add a fallback.
• make it possible to use keyboard input within the client
• make it possible to stream into the client (e.g. journalctl -u ssh.service | ./client)
• the server may die during transfer; make sure that the client handles that gracefully and tells the user that the server disconnected

Additionally, I'd suggest the following exercises for the server:

• use read and write as suggested for the symmetry
• advanced: investigate the POSIX library whether there is a function that can reduce the overhead (this may depend on whether server is streaming to stdout or into a file)

• Toby Speight: Is sendfile() portable across Unix-like OSes? I thought it was a Linux-only syscall.
• Zeta: @TobySpeight: No, it's not, and I should probably edit my last sentence to say "investigate the Linux syscalls". Linux-only is probably not an issue for OP though, given that they read "The Linux Programming Interface", but something to keep in mind.

Answer 2

Overall you're doing a good job of error checking. Keep that up - I see too much production code that doesn't.

In the signature to fatal, make message a const char *. Similar for get_data(argv).

Consider making a header shared with the server and client, and moving SOCKET_PATH into it.

If those 8 space indentations are caused by hard tabs, use soft (space) indentation instead. If they're actually 8 spaces, reduce them to 4.

Rather than sizeof on the type like

sizeof(struct sockaddr_un)

prefer sizeof on the variable, in this case socket_addr. It will remain true if the type of the variable changes, and I consider it more visually consistent in most expressions.

The second argument to listen is unclear unless you read the documentation or have a good memory. For long function signatures, I typically comment the parameter name with a parameter on each line; but here that's slightly overkill and the alternative is to declare an int backlog = 8 and pass that in.

This:

 fatal("Cannot accept connections");
 sleep(1);

probably intended to use perror instead of fatal.

Whereas a lot of C world unfortunately considers in-predicate mutation like this to be idiomatic:

while ((numread = read(connect_fd, buffer, BUFFER_LEN)) > 0)

it's a pain to read and debug. Instead write

while (1) {
 ssize_t numread = read(...);
 if (numread < 1)
 break;
}

printf is not the simplest API contract to use when all you want to do is print a string here: printf("%s", buffer). Instead consider fwrite. This will also remove the need for you to insert null termination.

I dearly hope that you're using at least C99, in which case these two lines:

 FILE* fp;
 fp = fopen(argv[1],"r");

should be combined.

• Super Tux: Hello, thanks for your answer! Is it okay to use type int for numread, when the return type of read() is ssize_t? What is the preferred type of numread as an return value of read()?
• Reinderien: By the documentation it should be ssize_t.
• user1686: Regarding fatal(), I would just say that's needlessly reinventing err()/errx() from err.h ...

Answer 3

Bug

When the server writes using printf("%s", buffer), that will output only the data up to the first 0円 character. To write all of the received data, including nulls, we need fwrite(buffer, numread, 1, stdout) instead. Don't forget to examine the return value!

Error handling

It's good to see errors considered throughout. We could mark fatal as [[noreturn]] if we're targeting current standard C (or use a macro to make do so conditional on compiler's standard version).

When we read, we should distinguish between reaching end-of-file (which is expected) and other, unexpected conditions. Log the latter.

Client may need to loop to transfer all the content using write() - don't assume that non-error return means everything was transferred.

Client buffer

Allocation of size+1 is more than required, since we only write to the first size elements. But would be better to use a fixed-size buffer as in the server, or (where possible), mmap() the input into process's address space.

Another problem with this allocation is that it means we only transfer the contents included in the file's size at the start of the transfer, and miss anything appended to it while the command is running. If we just loop read() into a buffer until we reach EOF, we magically get the full contents at the end of execution.

Minor concerns

Internal functions should be declared with static linkage.

return 0; is implicit at the end of main(), unlike other functions, so that line can be omitted from both programs.

memset() is a dead write (or would be, if we looped to read all of the input).

sleep() calls seem to be pointless.

Answer 4

Dubug

In the get_data function, this line is repeated twice for 2 different reasons:

fatal("Error determining file size");

If a user encounters this error message when running the code, it would be helpful to know which reason caused the error by making the error message unique. For example, you could add a unique word to the different fatal calls:

fatal("Error determining file size via fseek");
fatal("Error determining file size via ftell");

Documentation

It would be nice to add header comments to each file summarizing the purpose of the code. For example, for client.c:

/*
The client requires an input file, and it sends the contents
of the file to server.
Describe the input file here.
*/