3
\$\begingroup\$

I'm implementing a simple HTTP server in C, and this is the first part of it. Basically, it takes a string containing a "raw" HTTP request, and parse it into a struct Request, in a more machine readable form.

It consists of 3 files: main.c, lib.h, and lib.c.

main.c

#include <stdio.h>

#include "lib.h"


int main(void) {
    char *raw_request = "GET / HTTP/1.1\r\n"
            "Host: localhost:8080\r\n"
            "Connection: keep-alive\r\n"
            "Upgrade-Insecure-Requests: 1\r\n"
            "Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8\r\n"
            "User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/604.5.6 (KHTML, like Gecko) Version/11.0.3 Safari/604.5.6\r\n"
            "Accept-Language: en-us\r\n"
            "DNT: 1\r\n"
            "Accept-Encoding: gzip, deflate\r\n"
            "\r\n"
            "Usually GET requests don\'t have a body\r\n"
            "But I don\'t care in this case :)";
    struct Request *req = parse_request(raw_request);
    if (req) {
        printf("Method: %d\n", req->method);
        printf("Request-URI: %s\n", req->url);
        printf("HTTP-Version: %s\n", req->version);
        puts("Headers:");
        struct Header *h;
        for (h=req->headers; h; h=h->next) {
            printf("%32s: %s\n", h->name, h->value);
        }
        puts("message-body:");
        puts(req->body);
    }
    free_request(req);
    return 0;
}

lib.h

#ifndef C11CODEREVIEW_LIB_H
#define C11CODEREVIEW_LIB_H

typedef enum Method {UNSUPPORTED, GET, HEAD} Method;

typedef struct Header {
    char *name;
    char *value;
    struct Header *next;
} Header;

typedef struct Request {
    enum Method method;
    char *url;
    char *version;
    struct Header *headers;
    char *body;
} Request;


struct Request *parse_request(const char *raw);
void free_header(struct Header *h);
void free_request(struct Request *req);

#endif //C11CODEREVIEW_LIB_H

lib.c

#include <stdlib.h>
#include <string.h>

#include "lib.h"

struct Request *parse_request(const char *raw) {
    struct Request *req = NULL;
    req = malloc(sizeof(struct Request));
    if (!req) {
        return NULL;
    }
    memset(req, 0, sizeof(struct Request));

    // Method
    size_t meth_len = strcspn(raw, " ");
    if (memcmp(raw, "GET", strlen("GET")) == 0) {
        req->method = GET;
    } else if (memcmp(raw, "HEAD", strlen("HEAD")) == 0) {
        req->method = HEAD;
    } else {
        req->method = UNSUPPORTED;
    }
    raw += meth_len + 1; // move past <SP>

    // Request-URI
    size_t url_len = strcspn(raw, " ");
    req->url = malloc(url_len + 1);
    if (!req->url) {
        free_request(req);
        return NULL;
    }
    memcpy(req->url, raw, url_len);
    req->url[url_len] = '\0';
    raw += url_len + 1; // move past <SP>

    // HTTP-Version
    size_t ver_len = strcspn(raw, "\r\n");
    req->version = malloc(ver_len + 1);
    if (!req->version) {
        free_request(req);
        return NULL;
    }
    memcpy(req->version, raw, ver_len);
    req->version[ver_len] = '\0';
    raw += ver_len + 2; // move past <CR><LF>

    struct Header *header = NULL, *last = NULL;
    while (raw[0]!='\r' || raw[1]!='\n') {
        last = header;
        header = malloc(sizeof(Header));
        if (!header) {
            free_request(req);
            return NULL;
        }

        // name
        size_t name_len = strcspn(raw, ":");
        header->name = malloc(name_len + 1);
        if (!header->name) {
            free_request(req);
            return NULL;
        }
        memcpy(header->name, raw, name_len);
        header->name[name_len] = '\0';
        raw += name_len + 1; // move past :
        while (*raw == ' ') {
            raw++;
        }

        // value
        size_t value_len = strcspn(raw, "\r\n");
        header->value = malloc(value_len + 1);
        if (!header->value) {
            free_request(req);
            return NULL;
        }
        memcpy(header->value, raw, value_len);
        header->value[value_len] = '\0';
        raw += value_len + 2; // move past <CR><LF>

        // next
        header->next = last;
    }
    req->headers = header;
    raw += 2; // move past <CR><LF>

    size_t body_len = strlen(raw);
    req->body = malloc(body_len + 1);
    if (!req->body) {
        free_request(req);
        return NULL;
    }
    memcpy(req->body, raw, body_len);
    req->body[body_len] = '\0';


    return req;
}


void free_header(struct Header *h) {
    if (h) {
        free(h->name);
        free(h->value);
        free_header(h->next);
        free(h);
    }
}


void free_request(struct Request *req) {
    free(req->url);
    free(req->version);
    free_header(req->headers);
    free(req->body);
    free(req);
}

Output:

Method: 1
Request-URI: /
HTTP-Version: HTTP/1.1
Headers:
                 Accept-Encoding: gzip, deflate
                             DNT: 1
                 Accept-Language: en-us
                      User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/604.5.6 (KHTML, like Gecko) Version/11.0.3 Safari/604.5.6
                          Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
       Upgrade-Insecure-Requests: 1
                      Connection: keep-alive
                            Host: localhost:8080
message-body:
Usually GET requests don't have a body
But I don't care in this case :)

Process finished with exit code 0

Question:

The

if (!foo) {
    free_request(req);
    return NULL;
}

structures are ugly! How can I improve it?

\$\endgroup\$
1
\$\begingroup\$

The if (!foo) { ... structures are ugly! How can I improve it?

What code does it actually OK, yet for a slighly reduced code, let free_request() return NULL;

if (!req->version) {
    return free_request(req);
}

Code could roll a void *SQ_malloc(void **, size_t) helper function as alternative for a bit more code golfing.

// req->url = malloc(url_len + 1);
// if (!req->url) {
//     free_request(req);
//     return NULL;
// }
if (SQ_malloc(&req->url, url_len + 1) == NULL) {
    return free_request(req);
}

Or pre-calc the needs for a number of members and return true on SQ2_malloc() failure:

if (SQ2_malloc(&req->url, url_len + 1) || //
    SQ2_malloc(&req->this_member, this_len + 1) || //
    SQ2_malloc(&req->that_member, that_len + 1)) {
  return free_request(req);
}

Other

Avoid undefined behavior

raw is not known to have a valid array access of at least 3.

// memcmp(raw, "GET", strlen("GET"))
strncmp(raw, "GET", strlen("GET"))
// or better
#define S_GET "GET"  
strncmp(raw, S_GET, sizeof S_GET - 1);

Avoid end-of-line assumption

raw += ver_len + 2; // move past <CR><LF> assumes the lines ended with "\r\n". It could have ended with just one of those. Robust code would consider that.

Testing NULL-ness

In free_header(struct Header *h), code does a if (h) { .... I recommend doing the same with free_request(struct Request *req), add if (req) { .... It allows more tolerant usage and follows the acceptability of free(NULL).

Use the size of the de-referenced variable

Rather than use the size of some structure, use the size of the de-referenced pointer. Easier to code right, review and maintain.

// header = malloc(sizeof(Header));
header = malloc(sizeof *header));
\$\endgroup\$
  • \$\begingroup\$ Nice advice! Just one thing: would if ((req->url = malloc(url_len + 1)) == NULL) { ... be better than if (SQ_malloc(&req->url, url_len + 1) == NULL) { ...? \$\endgroup\$ – nalzok Feb 27 '18 at 0:35
  • 1
    \$\begingroup\$ @SunQingyao Both ideas about the same. For me, I like bool SQ_malloc(void **, size_t) as it is easier to chain. \$\endgroup\$ – chux Feb 27 '18 at 1:39
1
\$\begingroup\$

When scanning headers. If you fail you deallocate the request but not the list of headers already created.

    if (!header) {
        req->headers = last;    // Make sure you clean up the headers.
        free_request(req);
        return NULL;
    }

    // STUFF
    if (!header->name) {
        free(header);            // Clean up the current header
        req->headers = last;     // Clean up the list of good headers
        free_request(req);
        return NULL;
    }

    // STUFF
    if (!header->value) {
        free(header);            // Clean up the current header
        req->headers = last;     // Clean up the list of good headers
        free_request(req);
        return NULL;
    }

HTTP

Comments about the HTTP.

URL

You don't store the URL in your request.

typedef struct Request {
    enum Method method;
    char *url;                // This is not a URL
    char *version;
    struct Header *headers;
    char *body;
} Request;

This is the path component of the URL. The HTTP request splits the host and the path before making the request. To get the URL you need to combine the "Host" head parameter with the path.

Not if your web-server is going to support multiple sites using the same machine (same IP address) it is worth putting the "Host" into the request body. As the first thing you want to do is decide which site you are going to serve.

Headers

Header need not be unique. There may be multiple "Accept" headers for example. To make this easy for your code to support your header object should allow multiple values for each header.

Body Length

When a body is passed. The HTTP standard requires that the body length is passed in the header.

4.4 Message Length The transfer-length of a message is the length of the message-body as it appears in the message; that is, after any transfer-codings have been applied. When a message-body is included with a message, the transfer-length of that body is determined by one of the following (in order of precedence):

1.Any response message which "MUST NOT" include a message-body (such as the 1xx, 204, and 304 responses and any response to a HEAD request) is always terminated by the first empty line after the header fields, regardless of the entity-header fields present in the message.

2.If a Transfer-Encoding header field (section 14.41) is present and has any value other than "identity", then the transfer-length is defined by use of the "chunked" transfer-coding (section 3.6), unless the message is terminated by closing the connection.

3.If a Content-Length header field (section 14.13) is present, its decimal value in OCTETs represents both the entity-length and the transfer-length. The Content-Length header field MUST NOT be sent if these two lengths are different (i.e., if a Transfer-Encoding

header field is present). If a message is received with both a
Transfer-Encoding header field and a Content-Length header field,
the latter MUST be ignored.

4.If the message uses the media type "multipart/byteranges", and the transfer-length is not otherwise specified, then this self- delimiting media type defines the transfer-length. This media type MUST NOT be used unless the sender knows that the recipient can parse it; the presence in a request of a Range header with multiple byte- range specifiers from a 1.1 client implies that the client can parse multipart/byteranges responses.

  A range header might be forwarded by a 1.0 proxy that does not
  understand multipart/byteranges; in this case the server MUST
  delimit the message using methods defined in items 1,3 or 5 of
  this section.

5.By the server closing the connection. (Closing the connection cannot be used to indicate the end of a request body, since that would leave no possibility for the server to send back a response.)

For compatibility with HTTP/1.0 applications, HTTP/1.1 requests containing a message-body MUST include a valid Content-Length header field unless the server is known to be HTTP/1.1 compliant. If a request contains a message-body and a Content-Length is not given, the server SHOULD respond with 400 (bad request) if it cannot determine the length of the message, or with 411 (length required) if it wishes to insist on receiving a valid Content-Length.

All HTTP/1.1 applications that receive entities MUST accept the "chunked" transfer-coding (section 3.6), thus allowing this mechanism to be used for messages when the message length cannot be determined in advance.

Messages MUST NOT include both a Content-Length header field and a non-identity transfer-coding. If the message does include a non- identity transfer-coding, the Content-Length MUST be ignored.

When a Content-Length is given in a message where a message-body is allowed, its field value MUST exactly match the number of OCTETs in the message-body. HTTP/1.1 user agents MUST notify the user when an invalid length is received and detected.

Otherwise the web server does not know where the next HTTP request starts. This is especially important when the HEADER Connection: keep-alive is passed as the connection from the client will not close and the next request will be written to the same connection.

\$\endgroup\$

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.