5
\$\begingroup\$

I have written TCP server client code to send data of any size (1 byte - 64KB) in a loop.

The client (A) sends data to the server (B). B then sends the same data back to A. A matches the sent data to the data received. If the data does not match, control flow will break from the while-loop. If the data matches, the loop will continue to 200K bytes and then break.

Server code (server.c):

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <sys/time.h>
#include <sys/types.h>
#include <unistd.h>
#include <sys/select.h>
#include <errno.h>
#include "init.h"

#define SIZE 220000

int main(int argc, char *argv[])
{
    unsigned int ret, accept_desc, n, ssd, c = 1, err;
    struct timeval tv;
    unsigned char *buf1;
    fd_set rfd;

    ssd = server_listen(argc, argv);
    n = ssd;
    tv.tv_sec = 0;
    tv.tv_usec = 1000;

    FD_ZERO(&rfd);
    FD_SET(ssd, &rfd);
    while (1) {
        FD_SET(accept_desc, &rfd);
        ret = select(n + 1, &rfd, NULL, NULL, NULL);
        if (ret < 0) {
            err = errno;
            perror("Error in select\n");
            error(err);
        }
        if (FD_ISSET(ssd, &rfd)) {
            //Accept
            accept_desc = server_accept(ssd);
            fprintf(stderr, "client %d is connected\n", accept_desc);
            if (n < accept_desc)
                n = accept_desc;
        }
        if (FD_ISSET(accept_desc, &rfd)) {
            c++;
            //Receive
            buf1 = receive(accept_desc, c);

            //Send
            send_str(accept_desc, buf1, strlen(buf1) + 1);

            fprintf(stderr, "sent %ld\n", strlen(buf1) + 1);
            free(buf1);
        }
    }
}

Client code (client.c):

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <strings.h>
#include <sys/socket.h>
#include <netdb.h> //for Sockaddr and hostent
#include <errno.h>
#include <sys/select.h>
#include <unistd.h>
#include <sys/queue.h>
#include <time.h>
#include <sys/types.h>
#include <netinet/in.h>
#include <netinet/tcp.h>
#include "init.h"

#define SIZE 220000

int main(int argc, char *argv[])
{
    unsigned int err, i = 0 , len = 0, j, k = 1, fd;
    struct timeval tv;
    unsigned char *buf1, *buf2;
    fd_set rfd;

    fd = client_connect(argc, argv);
    tv.tv_sec = 0;
    tv.tv_usec = 1000;

    FD_ZERO(&rfd);
    FD_SET(0, &rfd);
    FD_SET(fd, &rfd);
    while(1) {
        FD_SET(0, &rfd);
        FD_SET(fd, &rfd);
        if ((select(fd + 1, &rfd, NULL, NULL, &tv) < 0)) {
            err = errno;
            perror("Select error");
            error(err);
        }

        if (FD_ISSET(fd, &rfd)) {

            buf2 = receive(fd, len + 1);

            if(strcmp(buf1, buf2) == 0){
                fprintf(stderr, "Match\n");
            } else {
                fprintf(stderr, "Unmatch %ld %ld\n", strlen(buf1), strlen(buf2));
                exit(1);
            }
            free(buf2);
            free(buf1);
            k = 1;
        }
        if(k == 1){
            buf1 = malloc(len + 2);
            j = 1;
            while(j <= i+1){
                buf1[j-1] = 'A';
                j++;
            }
            buf1[j-1] = 0;
            i++;
            len++;
            send_str(fd, buf1, strlen(buf1) + 1);
            k = 0;

        }
        if(len >= 200000)
            exit(1);
    }
}

Initial definition (init.c):

#include "init.h"
#include <stdio.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <netinet/in.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <arpa/inet.h>
#include <errno.h>
#include <sys/select.h>
#include <netinet/tcp.h>
#include <netdb.h>
#include <netinet/in.h>
#include <time.h>

#define MAX_CLIENT 20000
#define SIZE 220000

struct hostent *server;
int count = 0, csd[MAX_CLIENT];

void error(int err)
{
    fprintf(stderr, "Error No. %d\n", err);
    exit(err);
}

unsigned char *receive(int csd, int size)
{
    unsigned int ret, err;
    unsigned char *buf = malloc(size), *buf1 = NULL;

    memset(buf, 0,size);
lp:
    ret = recv(csd, buf, size, 0);

    if(buf[ret-1] > 0) {
        buf1 = malloc(size);
        strcpy(buf1, buf);
        memset(buf, 0, size);
        goto lp;
    }
    if (ret < 0) {
        err = errno;
        perror("Error in receive\n");
        error(err);
    } else if (ret == 0) {
        fprintf(stderr, "client disconnected\n");
        close(csd);
    } else {
        if(buf1 != NULL) {
            strcat(buf1, buf);
            free(buf);
            fprintf(stderr, "buf: %ld\n", strlen(buf));
            return buf1;
        }
        return buf;
    }

}

void send_str(int csd, unsigned char *buf, int size)
{
    unsigned int ret, err;

    ret = send(csd, buf, size, 0);
    if (ret < 0) {
        err = errno;
        perror("Error in send\n");
        error(err);
    } else if(ret == 0)
        fprintf(stderr, "Nothing to send\n");
}

int server_accept(int ssd)
{
    int accept_desc, err;
    struct sockaddr_in cli_addr;
    socklen_t cli_addr_len;

    accept_desc = accept(ssd, (struct sockaddr *)&cli_addr,
                  (socklen_t *)&cli_addr_len);
    if (accept_desc < 0) {
        err = errno;
        perror("Error in accept\n");
        error(err);
    }

    return accept_desc;
}

int get_option1(int argc, char **argv)
{
    int portno = 0, err, opt;

    while ((opt = getopt(argc, argv, "p:i:")) != -1) {
        switch (opt) {
        case 'p':
            portno = atoi(optarg);
            memset(&optarg, 0, strlen(optarg));
            break;
        case 'i':
            server = gethostbyname(optarg);
            memset(&optarg, 0, strlen(optarg));
            break;

        default: /* '?' */
            err = errno;
            fprintf(stderr, "Usage: %s\n", argv[0]);
            error(err);;
        }
    }
    if (portno == 0) {
        fprintf(stderr, "Assign port number\n");
        exit(1);
    }
    printf("portno = %d optind = %d\n", portno, optind);

    return portno;
}


int client_connect(int argc, char **argv)
{
    int fd1, err, ret, flag = 1, portno;
    struct sockaddr_in serv_addr;

    portno = get_option1(argc, argv);

    if(server == NULL) {
        fprintf(stderr, "Assign IP\n");
        exit(1);
    }

    fd1 = socket(AF_INET, SOCK_STREAM, 0);
    //fd1 = socket(AF_INET, SOCK_DGRAM, 0);
    if (fd1 == -1) {
        err = errno;
        perror("ERROR on creating Socket\n");
        error(err);
    }
    fprintf(stderr, "%d\n", fd1);
    fprintf(stderr, "socket created successfully.\n");
    bzero((char *) &serv_addr, sizeof(serv_addr));
    serv_addr.sin_family = AF_INET;
    bcopy((char *) server->h_addr, (char *)&serv_addr.sin_addr.s_addr,
    server->h_length);
    serv_addr.sin_port = htons(portno);
//Setsockopt
    ret = setsockopt(fd1, IPPROTO_TCP, TCP_NODELAY,
            (char *) &flag, sizeof(int));
    if (ret != 0) {
        err = errno;
        perror("ERROR :");
        error(err);
    }
//Connect
    if (connect(fd1, (struct sockaddr *)&serv_addr,
            sizeof(serv_addr)) == -1) {
        err = errno;
        perror("ERROR in Connecting");
        error(err);
    }
    printf("Client connected Successfully\n");

    return fd1;
}

int server_listen(int argc, char **argv)
{
    int ssd, err, flag = 1, ret, portno;
    size_t addrlen;
    struct sockaddr_in serv_addr;

    portno = get_option1(argc, argv);
//Socket
    ssd = socket(AF_INET, SOCK_STREAM, 0);
    //ssd = socket(AF_INET, SOCK_DGRAM, 0);
    if (ssd < 0) {
        err = errno;
        perror("Error in socket");
        error(err);
    }
//Setsockopt
    ret = setsockopt(ssd, IPPROTO_TCP, TCP_NODELAY,
            (char *) &flag, sizeof(int));
    if (ret != 0) {
        err = errno;
        perror("ERROR :");
        error(err);
    }
//Socket family
    serv_addr.sin_family = AF_INET;
    serv_addr.sin_addr.s_addr = INADDR_ANY;
    serv_addr.sin_port = htons(portno);
    bzero(&serv_addr.sin_zero, sizeof(serv_addr.sin_zero));
    addrlen = sizeof(serv_addr);
//Bind
    ret = bind(ssd, (struct sockaddr *)&serv_addr, addrlen);
    if (ret < 0) {
        err = errno;
        perror("Error in binding");
        error(err);
    }
    fprintf(stderr, "Bind completed\n");
//Listen
    ret = listen(ssd, MAX_CLIENT);
    if (ret < 0) {
        err = errno;
        perror("Error in listen");
        error(err);
    }
    fprintf(stderr, "listen\n");

    return ssd;
}

Initial declaration (init.h):

int server_listen(int argc, char **argv);

int client_connect(int argc, char **argv);

unsigned char *receive(int csd, int size);

void send_str(int csd, unsigned char *buf, int size);

int server_accept(int ssd);

void error(int err);

Makefile:

all:
    gcc -Wunused-variable server.c init.c -o s
    gcc -Wunused-variable client.c init.c -o c

server:
    ./s -p 12000

client:
    ./c -p 12000 -i 127.0.0.1

What I can I improve in this code?

Improve this question
\$\endgroup\$
4
  • \$\begingroup\$ So, if I read your intro correctly: you have an echoing server, and the client compares the if the echo matches the message that was send? \$\endgroup\$
    – Maarten Bodewes
    Jun 6, 2018 at 13:14
  • \$\begingroup\$ Yes, it's loopback \$\endgroup\$
    – ASid13
    Jun 6, 2018 at 13:17
  • 1
    \$\begingroup\$ @Adilsiddiqui There is still "1 byte - 64K+ bytes)" in the question text. I presumed KiB because I didn't see a constant for 64 KiB or 64KB in the code (and thus I went for 64Ki in the title, as a common boundary). Maybe clean up the question text. \$\endgroup\$
    – Maarten Bodewes
    Jun 6, 2018 at 14:03
  • 1
    \$\begingroup\$ Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Vogel612
    Jun 10, 2018 at 10:36

2 Answers 2

Reset to default
3
\$\begingroup\$

When writing a header file, either use: #pragma once or include a multi include guard similar to:

#ifndef INIT_H
#define INIT_H

int server_listen(int argc, char **argv);

int client_connect(int argc, char **argv);

unsigned char *receive(int csd, int size);

void send_str(int csd, unsigned char *buf, int size);

int server_accept(int ssd);

void error(int err);

#endif // INIT_H

There needs to be a clear delineation between code for the server and code for the client. Having the header file include prototypes for both applications is poor layout of the code (and in large projects will lead to 'very hard to debug' problems)

The goto indicates very poor code organization, so the code should be re-organized for flow. Th goto turns the code into spaghetti

In the Makefile, the rules all server and client do not actually produce files with those names, so there should be a statement:

.PHONY: all client server

When calling any of the heap allocation functions: malloc calloc realloc, always check (!=NULL) the returned value to assure the operation was successful.

The error() function is not adding any information to the error handling, Sugest eliminating that function,

The posted code, in the call to strcmp() is comparing a signed to a unsigned string. This will result in erronous results. Suggest both strings be either signed or both unsigned

when compiling, always enable the warnings, then fix those warnings. For gcc at a minmum use: -Wall -Wextra -Wconversion -pedantic -sttd=gnu11

The compiler can handle many many kinds of 'raggedness' in the layout of the code, However, humans can be easily confused by such code. In general, code needs to be written such that it is easily read, even by a persn that is not fully familiar with the (in this case) C programming language. Therefore, suggest:

  1. insert a single blank line around if else while do..while switch case default code blocks
  2. insert 2 or 3 blank lines between functions (be consistent)
  3. insert an 'appropriate' horizontal space: inside parens, inside braces, after commas, after semicolons, around C operators
  4. Variable (and parameter) names should indicate usage or content (or better, both)
  5. for ease of readability and understanding, follow the axiom: *only one statement per line and (at most) on variable declaration per statement.

Regarding: ret = recv(csd, buf, size, 0); and the several if/else code blocks that follow it: there are several logic errors, including:

if(buf1 != NULL) {
        strcat(buf1, buf);
        free(buf);
        fprintf(stderr, "buf: %ld\n", strlen(buf));
        return buf1;
}

Which seems to be checking if the call to malloc() was successful or not, but that is too late, as the pointer buf1 has already been dereferenced by the code:

if(buf[ret-1] > 0) {
    buf1 = malloc(size);
    strcpy(buf1, buf);    // <--
    memset(buf, 0, size);
    goto lp;
}

This statement:

err = errno;

is assigning a unsigned to a signed variable which can result is loss of the actual 'sign' of the value

The function: select() changes the contents of the last parameter (the timeout) so the contents of the last parameter needs to be re-set before each call to select()

Before calling exit(), clean up by passing allocated memory to free() and closing an open file descriptors. It is a poor practice to leave the sloppieness for the OS to cleanup

Calling strlen() on data received is a very poor practice as there may be a NUL byte buried in that data, which would result in an erronous value being returned

Regarding:

if ((select(fd + 1, &rfd, NULL, NULL, &tv) < 0)) {

This and the following code fails to check for a 'timeout' having occurred

It is very poor practice to include header files those contents are not being used. Suggest removing the #include statements for header files that are not used.

There is nothing in the Makefile to cause the executables s and c to be executed, certainly nothing to cause the executables to be executed at the same time.

Improve this answer
\$\endgroup\$
0
4
\$\begingroup\$

Design

It is unclear what the code does from context. That's never good. It seems like it is testing a server with a growing buffer containing A. This should be clear from the function names.

Functions such as:

int client_connect(int argc, char **argv)

should be in client.c. There is no reason to mix client / server code in a separate source file. This function will only ever perform client related functionality.

Functions such as:

int client_connect(int argc, char **argv)

should not both parse the command line arguments and perform the required functionality. It is always important to not mix user interface with the main functionality.

In the end you want to write Unit tests to test the functionality without touching the user interface. And you want to test the input validation and parsing without actually performing the functionality as well.

Code style

if (FD_ISSET(fd, &rfd)) {

...

if(k == 1){

Be consistent throughout the code and run your code through a code beautifier. Most people prefer to add a space before the brace {.

Although I won't go into a flame war on style here, most people don't go for K&R (or Egyptian) style braces for C code. What you should not do is to mix both of them in the code as you are doing now. Again, your style should be consistent throughout the code.

The naming of your variables is off:

 unsigned char *buf1, *buf2;

would never even get through a code review that I perform. OK, you need two buffers, but what are they used for?

Shortening variable names is OK (nobody will mistake buf for anything but buffer). However, people should not have to look up the usage of the variable name just to see what it does. Or put differently: if you think you need to explain your variable using a comment then you may need to rename your variable.

Your code in main seems to rely on state in k and in a function call. If you desire specific state you should implement a state machine. For this state you should define constants - meaning #define or const values - with good names (e.g. STATE_FILL and STATE_RECEIVE) and a single variable with a better name than just k.

You should never mix creation of the contents of the message with the receiving of the message. If you need to do this in a loop then you should create two functions for it; the while loop itself should consist of two or three lines.

(And you should certainly not do this in the while loop. And you certainly don't want two state variables control this through two if statements.)

In time, I presume that creation of the message is performed before it is received by the client. It is much better to structure the code in the same order as it is executed. I've gone as far as to explicitly prefix functions with step1ReceiveChallenge and step2SendChallenge (etc.) when an explicit order should be adhered to.

while(1)

You mean while (true) or even while (NO_EXIT_OR_ERROR) I suppose.

The use of the literals -1, 0, 1 and 2 is generally accepted if the meaning is clear from the context. However, I don't think this is always the case in your code.

There is an unholy mix of i, j and len in your client code. Short names are OK but not when used like this. Variables should have clear names and variables should not be reused.

C is fortunately zero based. Even VB is zero based nowadays (because of .NET). Use of one-based variables and <= x will immediately expose you as a beginner. Use >= 0 and < x and start your indices, such as j, with 0:

while(j <= i+1){
    buf1[j-1] = 'A';
    j++;
}

Comments

More comments are required. At least you need to explain what is being done if it is not directly clear from the code itself. Take this code:

    if(k == 1){
        buf1 = malloc(len + 2);
        j = 1;
        while(j <= i+1){
            buf1[j-1] = 'A';
            j++;
        }
        buf1[j-1] = 0;
        i++;
        len++;
        send_str(fd, buf1, strlen(buf1) + 1);
        k = 0;
    }

Would you be able to explain easily what it is supposed to do.

Comments should be well indented to the same line as the code. They should contain a space after the // line. You do not seem to use end-of-line comments which is good.

Comments that indicate a section in the code could start with an indication, like // --- listen for next transmission from client. This is of course mainly useful if other comments that describe a single line are added (see previous point).

Security

Beware that functions such as strlen should be treated with the proper respect. You should always make sure that you keep a byte at the end set to zero if you operate on input. Otherwise somebody can trick you into a buffer overflow attack.

Finally

if(len >= 200000)
    exit(1);

wait... what? Why? And why would you do this after receiving that many bytes? And why is 200000 a literal instead of a constant? Where are the braces?

The end was in sight, but now you were just getting sloppy.

200000 is probably something you want to configure from the user interface, possibly by an optional parameter with a default value. Like ping does.

Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ Minor: In C, -1, 0, 1 and 2 are identified as constants, not literals. C has 2 literals: string literals and compound literals. \$\endgroup\$
    – chux - Reinstate Monica
    Jun 6, 2018 at 20:01

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.