[vlc-devel] [PATCH] network: implement an API for HTTP requests.

[Denis Charmet]

Hi,

Since it's quite redundant with access/http, it would be good to avoid
code duplication.

Le lundi 03 février 2014 à 07:07:48, Felix Abecassis a écrit :
> +/* Read a line (ended by \r\n) from an HTTP response. */
> +static char* http_readline( vlc_object_t *p_this, int fd )
why not net_Get?
> +{
> +    size_t size = 64;
> +    char *str = malloc(sizeof(*str) * size);
no check
> +
> +    char c;
> +    size_t i = 0;
> +    while (net_Read(p_this, fd, NULL, &c, 1, false) > 0)
> +    {
> +        if (i == size - 1)
> +        {
> +            /* Ran out of space for storing the line, grow the buffer. */
> +            size *= 2;
> +            str = realloc(str, sizeof(*str) * size);
> +            if (!str)
> +                return NULL;
> +        }
> +        str[i++] = c;
> +        if (c == '\n')
> +            break;
> +    }
> +
> +    if (i == 0)
> +    {
> +        free(str);
> +        return NULL;
> +    }
> +
> +    str[i] = '\0';
> +    return str;
> +}
> +
> +/* Concatenate the given key-value pair to string headers. */
> +static char *http_add_header(char *headers, const char *key, const char *value)
> +{
> +    char *new_headers;
> +    int ret = asprintf(&new_headers, "%s%s: %s\r\n", headers, key, value);
> +    free(headers);
> +    return ret >= 0 ? new_headers : NULL;
> +}
> +
> +/* Split a header line with format "Key: Value" */
> +static int http_split_header_line(const char *line, char** key, char **value)
> +{
> +    *key = NULL;
> +    *value = NULL;
> +
> +    const char *key_begin = line;
> +    const char *key_end = strchr(key_begin, ':');
> +    *key = strndup(key_begin, key_end - key_begin);
> +    if (!*key)
> +        goto error;
> +
> +    const char *value_begin = key_end + 1;
Never trust a source if there is no ':' the following dereference is likely to segfault.
> +    while (*value_begin == ' ')
You may also skip potential '\t'
> +        ++value_begin;
> +    const char *value_end = strchr(value_begin, '\r');
> +    if (!value_end)
> +        goto error;
> +    /* If value_end == value_begin, the field is empty, this is valid. */
> +    if (value_end != value_begin)
> +    {
> +        *value = strndup(value_begin, value_end - value_begin);
> +        if (!*value)
> +            goto error;
> +    }
> +
> +    return 0;
> +
> +error:
> +    free(*key);
> +    free(*value);
> +    return -1;
> +}
> +
> +static vlc_http_response_t *http_get_response(vlc_object_t *p_this, int fd)
> +{
> +    vlc_http_response_t *response = calloc(1, sizeof(*response));
> +    if (!response)
> +        return NULL;
> +
> +    vlc_http_status_line_t *status_line = &response->status_line;
> +    vlc_http_header_t *header = &response->header;
> +    vlc_http_body_t *body = &response->body;
> +
> +    /* Parse status line. */
> +    char *line = http_readline(p_this, fd);
> +    int ret = sscanf(line, "HTTP/1.%d %3d", &status_line->http_version, &status_line->status_code);
You'll have to modify when HTTP/2.0 so why not already forsee the case?
> +    free(line);
> +    if (ret != 2)
> +    {
> +        msg_Err(p_this, "Received invalid HTTP status line.");
> +        goto error;
> +    }
> +
> +    /* Parse headers, line by line. */
> +    while ((line = http_readline(p_this, fd)))
> +    {
> +        /* Line with only \r\n: end of headers. */
> +        if (!strncmp(line, "\r\n", 2))
is it worth to call strncmp when you could test just line[0] == '\r' &&
line[1] == '\n' if there are 2 chars
> +        {
> +            free(line);
> +            break;
> +        }
> +
> +        /* Extract the key-value pair from the raw line. */
> +        char *key = NULL;
> +        char *value = NULL;
> +        ret = http_split_header_line(line, &key, &value);
> +        free(line);
> +        if (ret < 0)
> +            goto error;
> +
> +        /* Grow the dictionary and add the new key-value pair. */
> +        header->dictionary = realloc(header->dictionary, sizeof(*header->dictionary) * (header->size + 1));
You shouldn't use realloc like that. It can return NULL but
header->dictionary will still be allocated and will be leaked.
Why not using the array macros already available in vlc_arrays.h
> +        if (!header->dictionary)
> +        {
> +            free(key);
> +            free(value);
> +            goto error;
> +        }
> +        header->dictionary[header->size].key = key;
> +        header->dictionary[header->size].value = value;
> +        ++header->size;
> +    }
> +
> +    /* Try to find Content-Length entry in the header */
> +    for (size_t i = 0; i < header->size; ++i)
> +    {
> +        const char *key = header->dictionary[i].key;
> +        const char *value = header->dictionary[i].value;
> +        if (!strncasecmp(key, "Content-Length", 14))
This could be done in the previous loop
> +        {
> +            /* Get the field value. */
> +            errno = 0;
> +            body->size = strtoul(value, NULL, 10);
> +            if (errno)
> +            {
> +                msg_Err(p_this, "Invalid Content-Length in HTTP response.");
> +                goto error;
> +            }
> +            break;
> +        }
> +    }
> +
> +    /* Read the body of the response. */
> +    if (body->size > 0)
> +    {
> +        /* We add a terminal \0 for debugging convenience. */
> +        body->data = malloc(body->size + 1);
Should debug stuff be in the core?
> +        if (!body->data)
> +            goto error;
> +
> +        size_t size = net_Read(p_this, fd, NULL, body->data, body->size, true);
> +        if (size < body->size)
> +            goto error;
> +
> +        char* end = (char*)body->data + body->size;
> +        *end = '\0';
> +    }
> +    return response;
> +
> +error:
> +    msg_Err(p_this, "Error in %s", __FUNCTION__);
> +    http_response_Delete(response);
> +    return NULL;
> +}
> +
> +typedef struct http_method_s
> +{
> +    vlc_http_method_t id;
> +    const char        *string;
> +} http_method_t;
> +
> +static const struct http_method_s http_methods[] =
> +{
> +    { VLC_HTTP_OPTIONS, "OPTIONS" },
> +    { VLC_HTTP_GET, "GET" },
> +    { VLC_HTTP_HEAD, "HEAD" },
> +    { VLC_HTTP_POST, "POST" },
> +    { VLC_HTTP_PUT, "PUT" },
> +    { VLC_HTTP_DELETE, "DELETE" },
> +    { VLC_HTTP_TRACE, "TRACE" },
> +    { VLC_HTTP_CONNECT, "CONNECT" },
> +    { VLC_HTTP_PATCH, "PATCH" },
> +};
> +#define HTTP_METHODS_COUNT (sizeof(http_methods)/sizeof(http_methods[0]))
> +
> +#define HTTP_EXPECT_CONTINUE_THRESHOLD 1024
> +
> +#undef http_request_Send
> +vlc_http_response_t *http_request_Send(vlc_object_t *p_this, vlc_url_t *url,
> +                                       vlc_http_request_t *request)
> +{
> +    vlc_http_request_line_t *request_line = &request->request_line;
> +    vlc_http_header_t *request_header = &request->header;
> +    vlc_http_body_t *request_body = &request->body;
> +
> +    char *header_string = NULL;
> +    vlc_http_response_t *response = NULL;
> +
> +    int fd = net_ConnectTCP(p_this, url->psz_host, url->i_port);
> +    if (fd < 0)
> +    {
> +        msg_Err(p_this, "Could not connect to %s:%d", url->psz_host, url->i_port);
> +        return NULL;
> +    }
> +
> +    /* Lookup the string corresponding to the HTTP method. */
> +    const char* method_string = NULL;
> +    for (unsigned i = 0; i < HTTP_METHODS_COUNT; ++i)
> +    {
> +        if (http_methods[i].id == request_line->method)
> +        {
> +            method_string = http_methods[i].string;
> +            break;
> +        }
> +    }
<nitpicking>
You could use a const char** array instead of a for loop and using methods as
index or reorder your methods by probability of use like GET first
</nitpicking>
> +    if (!method_string)
> +        goto end;
> +
> +    /* Create the header string. */
> +    int ret;
> +    header_string = strdup("");
Why strdup("") when you could asprintf("%s %s HTTP/1.%d",...)
And you didn't check for result.

> +    for (size_t i = 0; i < request_header->size; ++i)
> +    {
> +        const char *key = request_header->dictionary[i].key;
> +        const char *value = request_header->dictionary[i].value;
> +        /* Discard unauthorized headers */
> +        if (!strncasecmp(key, "Content-Length", 14)
> +            || !strncasecmp(key, "Connection", 10))
Discard "\r\n" :)
> +        {
> +            msg_Err(p_this, "Unauthorized HTTP header: %s %s (overwritten by VLC)", key, value);
> +            continue;
> +        }
> +        /* Concatenate new key-value entry to the string. */
> +        header_string = http_add_header(header_string, key, value);
> +        if (!header_string)
> +            goto end;
> +    }
> +    /* If the body is larger than the threshold, we first ask the
> +       server if the headers are acceptable using Expect: 100-continue.
> +       This is only for HTTP 1.1. */
> +    bool use_expect_continue = request_line->http_version >= 1 && request_body->size >= HTTP_EXPECT_CONTINUE_THRESHOLD;
> +    if (use_expect_continue)
> +        if (!(header_string = http_add_header(header_string, "Expect", "100-continue")))
Maybe make Expect a forbidden header like "Content-Length" and
"Connection"
> +            goto end;
> +
> +    /* Force "Connection: close" mode. */
> +    if (!(header_string = http_add_header(header_string, "Connection", "close")))
Suddenly I wonder what happens when you have "Connection: close" and
"Expect: 100-continue"
In any case there might be cases where you could want the connection to
stay alive but fair enough for now.
> +        goto end;
> +
> +    /* Send request line and header. */
> +    ret = net_Printf(p_this, fd, NULL,
> +                     "%s %s HTTP/1.%d\r\n"
> +                     "%s"
> +                     "Content-Length: %zu\r\n"
> +                     "\r\n",
> +                     method_string, request_line->uri, request_line->http_version,
> +                     header_string, request_body->size);
> +    if (ret == -1)
> +        goto end;
> +
> +    if (use_expect_continue)
> +    {
> +        vlc_http_response_t *expect_response = http_get_response(p_this, fd);
> +        if (!expect_response) /* Error while reading the response. */
> +            goto end;
> +        int status_code = expect_response->status_line.status_code;
> +        http_response_Delete(expect_response);
> +        if (status_code != 100) /* Server did not accept the headers. */
> +        {
> +            msg_Err(p_this, "\"Expect: 100-continue\" request denied (status: %d).", status_code);
> +            goto end;
> +        }
> +    }
> +
> +    /* Send body. */
> +    if (request_body->size > 0)
> +    {
> +        ret = net_Write(p_this, fd, NULL, request_body->data, request_body->size);
> +        if (ret == -1)
> +        {
> +            msg_Err(p_this, "Could not send HTTP request body.");
> +            goto end;
> +        }
> +    }
> +
> +    response = http_get_response(p_this, fd);
> +
> +end:
> +    free(header_string);
> +    net_Close(fd);
> +    return response;
> +}
> +
> +void http_response_Delete(vlc_http_response_t *response)
> +{
> +    if (!response)
> +        return;
> +
> +    vlc_http_header_t *header = &response->header;
> +    vlc_http_body_t *body = &response->body;
> +
> +    for (size_t i = 0; i < header->size; ++i)
> +    {
> +        char *key = (char*)header->dictionary[i].key;
> +        char *value = (char*)header->dictionary[i].value;
> +        free(key);
> +        free(value);
> +    }
> +    free(header->dictionary);
> +    free(body->data);
> +    free(response);
> +}

Regards

-- 
Denis Charmet - TypX
Le mauvais esprit est un art de vivre