Skip to content

Commit

Permalink
Misc cleanup of aws-c-http (#30)
Browse files Browse the repository at this point in the history 
- Remove "TODO" comments that exist as tasks on our planning board.
- Write unit tests to address some TODO comments.
- Check for overflow when summing numbers.
- Don't log contents of bad data at the ERROR level, log it at DEBUG. This is to avoid log injection attacks.
  • Loading branch information
@graebm
graebm authored Apr 9, 2019
1 parent 753650c commit eeb0597
Show file tree
Hide file tree
Showing 6 changed files with 325 additions and 80 deletions.
12 changes: 6 additions & 6 deletions source/connection.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ static struct aws_http_connection *s_connection_new(
AWS_LS_HTTP_CONNECTION,
"static: Failed to insert slot into channel %p, error %d (%s).",
(void *)channel,
err,
aws_error_name(err));
aws_last_error(),
aws_error_name(aws_last_error()));
goto error;
}

Expand Down Expand Up @@ -143,8 +143,8 @@ static struct aws_http_connection *s_connection_new(
AWS_LS_HTTP_CONNECTION,
"static: Failed to setting HTTP handler into slot on channel %p, error %d (%s).",
(void *)channel,
err,
aws_error_name(err));
aws_last_error(),
aws_error_name(aws_last_error()));

goto error;
}
Expand Down Expand Up @@ -527,8 +527,8 @@ int aws_http_client_connect(const struct aws_http_client_connection_options *opt
AWS_LOGF_ERROR(
AWS_LS_HTTP_CONNECTION,
"static: Failed to initiate socket channel for new client connection, error %d (%s).",
err,
aws_error_name(err));
aws_last_error(),
aws_error_name(aws_last_error()));

goto error;
}
Expand Down
120 changes: 85 additions & 35 deletions source/connection_h1.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ enum stream_type {

enum stream_outgoing_state {
STREAM_OUTGOING_STATE_HEAD,
STREAM_OUTGOING_STATE_BODY, /* TODO: support 100-continue */
STREAM_OUTGOING_STATE_BODY,
STREAM_OUTGOING_STATE_DONE,
};

Expand Down Expand Up @@ -278,7 +278,7 @@ static int s_stream_scan_outgoing_headers(
size_t num_headers,
size_t *out_header_lines_len) {

*out_header_lines_len = 0;
size_t total = 0;

for (size_t i = 0; i < num_headers; ++i) {
struct aws_http_header header = header_array[i];
Expand All @@ -288,20 +288,25 @@ static int s_stream_scan_outgoing_headers(
if (header.name.len > 0) {
name_enum = aws_http_str_to_header_name(header.name);
} else {
AWS_LOGF_ERROR(AWS_LS_HTTP_STREAM, "static: No name set for header[%zu].", i);
AWS_LOGF_ERROR(
AWS_LS_HTTP_CONNECTION,
"id=%p: Failed to create stream, no name set for header[%zu].",
(void *)stream->base.owning_connection,
i);
return aws_raise_error(AWS_ERROR_INVALID_ARGUMENT);
}

switch (name_enum) {
case AWS_HTTP_HEADER_CONTENT_LENGTH:
case AWS_HTTP_HEADER_TRANSFER_ENCODING:
/* TODO: actually process the values in these headers*/
stream->has_outgoing_body = true;

if (!stream->base.stream_outgoing_body) {
AWS_LOGF_ERROR(
AWS_LS_HTTP_STREAM,
"static: '" PRInSTR "' header specified, but body-streaming callback is not set.",
AWS_LS_HTTP_CONNECTION,
"id=%p: Failed to create stream, '" PRInSTR
"' header specified but body-streaming callback is not set.",
(void *)stream->base.owning_connection,
AWS_BYTE_CURSOR_PRI(header.name));

return aws_raise_error(AWS_ERROR_INVALID_ARGUMENT);
Expand All @@ -312,11 +317,22 @@ static int s_stream_scan_outgoing_headers(
}

/* header-line: "{name}: {value}\r\n" */
*out_header_lines_len += header.name.len + 2 + header.value.len + 2;

/* TODO: check for overflows anywhere we do addition/subtraction? */
int err = 0;
err |= aws_add_size_checked(header.name.len, total, &total);
err |= aws_add_size_checked(header.value.len, total, &total);
err |= aws_add_size_checked(4, total, &total); /* ": " + "\r\n" */
if (err) {
AWS_LOGF_ERROR(
AWS_LS_HTTP_CONNECTION,
"id=%p: Failed to create stream, header size calculation produced error %d (%s)'",
(void *)stream->base.owning_connection,
aws_last_error(),
aws_error_name(aws_last_error()));
return AWS_OP_ERR;
}
}

*out_header_lines_len = total;
return AWS_OP_SUCCESS;
}

Expand All @@ -339,7 +355,10 @@ static void s_write_headers(struct aws_byte_buf *dst, const struct aws_http_head

struct aws_http_stream *s_new_client_request_stream(const struct aws_http_request_options *options) {
if (options->uri.len == 0 || options->method.len == 0) {
AWS_LOGF_ERROR(AWS_LS_HTTP_CONNECTION, "static: Invalid options, cannot create client request.");
AWS_LOGF_ERROR(
AWS_LS_HTTP_CONNECTION,
"id=%p: Cannot create client request, options are invalid.",
(void *)options->client_connection);
aws_raise_error(AWS_ERROR_INVALID_ARGUMENT);
return NULL;
}
Expand Down Expand Up @@ -368,24 +387,48 @@ struct aws_http_stream *s_new_client_request_stream(const struct aws_http_reques

/**
* Calculate total size needed for outgoing_head_buffer, then write to buffer.
* The head will look like this:
* request-line: "{method} {uri} {version}\r\n"
* header-line: "{name}: {value}\r\n"
* head-end: "\r\n"
*/
size_t request_line_len = options->method.len + 1 + options->uri.len + 1 + version.len + 2;

size_t header_lines_len;
int err = s_stream_scan_outgoing_headers(stream, options->header_array, options->num_headers, &header_lines_len);
if (err) {
goto error;
/* errors already logged by scan_outgoing_headers() function */
goto error_scanning_headers;
}

/* request-line: "{method} {uri} {version}\r\n" */
size_t request_line_len = 4; /* 2 spaces + "\r\n" */
err |= aws_add_size_checked(options->method.len, request_line_len, &request_line_len);
err |= aws_add_size_checked(options->uri.len, request_line_len, &request_line_len);
err |= aws_add_size_checked(version.len, request_line_len, &request_line_len);

/* head-end: "\r\n" */
size_t head_end_len = 2;

size_t head_total_len = request_line_len + header_lines_len + head_end_len;
size_t head_total_len = request_line_len;
err |= aws_add_size_checked(header_lines_len, head_total_len, &head_total_len);
err |= aws_add_size_checked(head_end_len, head_total_len, &head_total_len);

if (err) {
AWS_LOGF_ERROR(
AWS_LS_HTTP_CONNECTION,
"id=%p: Failed to create request, size calculation had error %d (%s).",
(void *)options->client_connection,
aws_last_error(),
aws_error_name(aws_last_error()));
goto error_calculating_size;
}

err = aws_byte_buf_init(&stream->outgoing_head_buf, stream->base.alloc, head_total_len);
if (err) {
goto error;
AWS_LOGF_ERROR(
AWS_LS_HTTP_CONNECTION,
"id=%p: Failed to create request, buffer initialization had error %d (%s).",
(void *)options->client_connection,
aws_last_error(),
aws_error_name(aws_last_error()));

goto error_initializing_buf;
}

bool wrote_all = true;
Expand Down Expand Up @@ -430,18 +473,19 @@ struct aws_http_stream *s_new_client_request_stream(const struct aws_http_reques

if (is_shutting_down) {
AWS_LOGF_ERROR(
AWS_LS_HTTP_STREAM,
"static: Connection is closed, cannot create " PRInSTR " request.",
AWS_BYTE_CURSOR_PRI(options->method));
AWS_LS_HTTP_CONNECTION,
"id=%p: Connection is closed, cannot create request.",
(void *)options->client_connection);

aws_raise_error(AWS_ERROR_HTTP_CONNECTION_CLOSED);
goto error;
goto error_connection_closed;
}

AWS_LOGF_DEBUG(
AWS_LS_HTTP_STREAM,
"id=%p: Created client request: " PRInSTR " " PRInSTR " " PRInSTR,
"id=%p: Created client request on connection=%p: " PRInSTR " " PRInSTR " " PRInSTR,
(void *)&stream->base,
(void *)options->client_connection,
AWS_BYTE_CURSOR_PRI(options->method),
AWS_BYTE_CURSOR_PRI(options->uri),
AWS_BYTE_CURSOR_PRI(aws_http_version_to_str(connection->base.http_version)));
Expand All @@ -453,8 +497,11 @@ struct aws_http_stream *s_new_client_request_stream(const struct aws_http_reques

return &stream->base;

error:
error_connection_closed:
aws_byte_buf_clean_up(&stream->outgoing_head_buf);
error_initializing_buf:
error_calculating_size:
error_scanning_headers:
aws_mem_release(stream->base.alloc, stream);
return NULL;
}
Expand All @@ -472,7 +519,14 @@ static void s_stream_destroy(struct aws_http_stream *stream_base) {
static void s_update_window_action(struct h1_connection *connection, size_t increment_size) {
int err = aws_channel_slot_increment_read_window(connection->base.channel_slot, increment_size);
if (err) {
/* TODO: log warning OR remove error code from aws_channel_slot_increment_read_window */
AWS_LOGF_ERROR(
AWS_LS_HTTP_CONNECTION,
"id=%p: Failed to increment read window, error %d (%s). Closing connection.",
(void *)&connection->base,
aws_last_error(),
aws_error_name(aws_last_error()));

s_shutdown_connection(connection, aws_last_error());
}
}

Expand Down Expand Up @@ -856,8 +910,8 @@ static void s_outgoing_stream_task(struct aws_channel_task *task, void *arg, enu
AWS_LS_HTTP_CONNECTION,
"id=%p: Failed to send message up channel, error %d (%s). Closing connection.",
(void *)&connection->base,
err,
aws_error_name(err));
aws_last_error(),
aws_error_name(aws_last_error()));

goto error;
}
Expand Down Expand Up @@ -914,8 +968,6 @@ static int s_decoder_on_request(
AWS_BYTE_CURSOR_PRI(*method_str),
AWS_BYTE_CURSOR_PRI(*uri));

/* TODO: Limit on lengths of incoming data https://httpwg.org/specs/rfc7230.html#attack.protocol.element.length */

/* Copy strings to internal buffer */
struct aws_byte_buf *storage_buf = &incoming_stream->incoming_storage_buf;
assert(storage_buf->capacity == 0);
Expand Down Expand Up @@ -976,8 +1028,6 @@ static int s_decoder_on_header(const struct aws_http_decoded_header *header, voi
AWS_BYTE_CURSOR_PRI(header->name_data),
AWS_BYTE_CURSOR_PRI(header->value_data));

/* TODO? how to support trailing headers? distinct cb? invoke same cb again? */

if (incoming_stream->base.on_incoming_headers) {
struct aws_http_header deliver = {
.name = header->name_data,
Expand Down Expand Up @@ -1263,8 +1313,8 @@ static int s_handler_process_read_message(
AWS_LS_HTTP_CONNECTION,
"id=%p: Message processing failed, error %d (%s). Closing connection.",
(void *)&connection->base,
err,
aws_error_name(err));
aws_last_error(),
aws_error_name(aws_last_error()));

goto error;
}
Expand All @@ -1282,8 +1332,8 @@ static int s_handler_process_read_message(
AWS_LS_HTTP_CONNECTION,
"id=%p: Failed to increment read window, error %d (%s). Closing connection.",
(void *)&connection->base,
err,
aws_error_name(err));
aws_last_error(),
aws_error_name(aws_last_error()));

goto error;
}
Expand Down
Loading

0 comments on commit eeb0597

Please sign in to comment.