Address TODOs

source/connection_manager.c

@@ -124,17 +124,16 @@ enum aws_http_connection_manager_count_type {
  * READY - connections may be acquired and released.  When the external ref count for the manager
  * drops to zero, the manager moves to:
  *
- * TODO: Seems like connections can still be release while shutting down.
  * SHUTTING_DOWN - connections may no longer be acquired and released (how could they if the external
- * ref count was accurate?) but in case of user ref errors, we simply fail attempts to do so rather
- * than crash or underflow.  While in this state, we wait for a set of tracking counters to all fall to zero:
+ * ref count was accurate?)
+ * While in this state, we wait for a set of tracking counters to all fall to zero:
  *
  *   pending_connect_count - the # of unresolved calls to the http layer's connect logic
  *   open_connection_count - the # of connections for whom the shutdown callback (from http) has not been invoked
  *   vended_connection_count - the # of connections held by external users that haven't been released.  Under correct
  *      usage this should be zero before SHUTTING_DOWN is entered, but we attempt to handle incorrect usage gracefully.
  *
- * While all the counter fall to zero and no outlife transition, connection manager will detroy itself.
+ * While all the counter fall to zero and no outlive transition, connection manager will detroy itself.
  *
  * While shutting down, as pending connects resolve, we immediately release new incoming (from http) connections
  *
@@ -1232,6 +1231,7 @@ int aws_http_connection_manager_release_connection(
         (void *)connection);
 
     aws_mutex_lock(&manager->lock);
+    AWS_FATAL_ASSERT(manager->state == AWS_HCMST_READY);
 
     /* We're probably hosed in this case, but let's not underflow */
     if (manager->internal_ref[AWS_HCMCT_VENDED_CONNECTION] == 0) {

source/h2_connection.c

@@ -2082,9 +2082,6 @@ static struct aws_http_stream *s_connection_make_request(
 
     struct aws_h2_connection *connection = AWS_CONTAINER_OF(client_connection, struct aws_h2_connection, base);
 
-    /* #TODO: http/2-ify the request (ex: add ":method" header). Should we mutate a copy or the original? Validate?
-     *  Or just pass pointer to headers struct and let encoder transform it while encoding? */
-
     struct aws_h2_stream *stream = aws_h2_stream_new_request(client_connection, options);
     if (!stream) {
         CONNECTION_LOGF(
@@ -2112,8 +2109,22 @@ static struct aws_http_stream *s_connection_make_request(
             aws_error_name(aws_last_error()));
         goto error;
     }
+    int error = 0;
+    struct aws_byte_cursor method;
+    AWS_ZERO_STRUCT(method);
+    struct aws_byte_cursor path;
+    AWS_ZERO_STRUCT(path);
+
+    error |= aws_http_message_get_request_method(stream->thread_data.outgoing_message, &method);
+    error |= aws_http_message_get_request_path(stream->thread_data.outgoing_message, &path);
+    AWS_ASSERT(!error);
 
-    AWS_H2_STREAM_LOG(DEBUG, stream, "Created HTTP/2 request stream"); /* #TODO: print method & path */
+    AWS_H2_STREAM_LOGF(
+        DEBUG,
+        stream,
+        "Created HTTP/2 request stream, method: " PRInSTR ". path: " PRInSTR "",
+        AWS_BYTE_CURSOR_PRI(method),
+        AWS_BYTE_CURSOR_PRI(path));
     return &stream->base;
 
 error:
@@ -2818,15 +2829,18 @@ static void s_gather_statistics(struct aws_channel_handler *handler, struct aws_
     struct aws_h2_connection *connection = handler->impl;
     AWS_PRECONDITION(aws_channel_thread_is_callers_thread(connection->base.channel_slot->channel));
 
-    /* TODO: Need update the way we calculate statistics, to account for user-controlled pauses.
-     * If user is adding chunks 1 by 1, there can naturally be a gap in the upload.
-     * If the user lets the stream-window go to zero, there can naturally be a gap in the download. */
     uint64_t now_ns = 0;
     if (aws_channel_current_clock_time(connection->base.channel_slot->channel, &now_ns)) {
         return;
     }
 
     if (!aws_linked_list_empty(&connection->thread_data.outgoing_streams_list)) {
+        /**
+         * For stream flow control stall and writing to stream over time, the stream will be removed from the
+         * outgoing_streams_list.
+         * For connection level flow control, as there could be streams waiting for response, we cannot mark the
+         * connection is inactive.
+         */
         s_add_time_measurement_to_stats(
             connection->thread_data.outgoing_timestamp_ns,
             now_ns,
@@ -2842,6 +2856,9 @@ static void s_gather_statistics(struct aws_channel_handler *handler, struct aws_
 
         connection->thread_data.incoming_timestamp_ns = now_ns;
     } else {
+        /**
+         * was inactive as no stream has data to write or waiting for data from the other side.
+         */
         connection->thread_data.stats.was_inactive = true;
     }
 

source/h2_decoder.c

@@ -1159,6 +1159,42 @@ static struct aws_h2err s_flush_pseudoheaders(struct aws_h2_decoder *decoder) {
     if (has_request_pseudoheaders) {
         /* Request header-block. */
         current_block->block_type = AWS_HTTP_HEADER_BLOCK_MAIN;
+        const struct aws_string *method_string = current_block->pseudoheader_values[PSEUDOHEADER_METHOD];
+        const struct aws_string *scheme_string = current_block->pseudoheader_values[PSEUDOHEADER_SCHEME];
+        const struct aws_string *authority_string = current_block->pseudoheader_values[PSEUDOHEADER_AUTHORITY];
+        const struct aws_string *path_string = current_block->pseudoheader_values[PSEUDOHEADER_PATH];
+
+        if (!method_string) {
+            DECODER_LOG(ERROR, decoder, "Request is missing :method.");
+            goto malformed;
+        }
+        if (!aws_strutil_is_http_token(aws_byte_cursor_from_string(method_string))) {
+            DECODER_LOG(ERROR, decoder, "Request method is invalid.");
+            DECODER_LOGF(
+                DEBUG,
+                decoder,
+                "Bad method is: '" PRInSTR "'",
+                AWS_BYTE_CURSOR_PRI(aws_byte_cursor_from_string(method_string)));
+            goto malformed;
+        }
+        if (aws_string_eq_c_str(method_string, "CONNECT")) {
+            if (scheme_string || path_string) {
+                /* RFC-9113 8.5 The ":scheme" and ":path" pseudo-header fields MUST be omitted for CONNECT method */
+                DECODER_LOG(ERROR, decoder, "CONNECT request has :path or :scheme that are must omitted.");
+                goto malformed;
+            }
+            if (!authority_string) {
+                DECODER_LOG(ERROR, decoder, "CONNECT request is missing :authority.");
+                goto malformed;
+            }
+        } else {
+            if (!scheme_string || !path_string) {
+                /* RFC-9113 8.3.1 All HTTP/2 requests MUST include exactly one valid value for the ":method", ":scheme",
+                 * and ":path" pseudo-header fields, unless they are CONNECT requests */
+                DECODER_LOG(ERROR, decoder, "Request missing :scheme or :path, which are required to have.");
+                goto malformed;
+            }
+        }
 
     } else if (has_response_pseudoheaders) {
         /* Response header block. */
@@ -1202,9 +1238,6 @@ static struct aws_h2err s_flush_pseudoheaders(struct aws_h2_decoder *decoder) {
         current_block->block_type = AWS_HTTP_HEADER_BLOCK_TRAILING;
     }
 
-    /* #TODO RFC-7540 8.1.2.3 & 8.3 Validate request has correct pseudoheaders. Note different rules for CONNECT */
-    /* #TODO validate pseudoheader values. each one has its own special rules */
-
     /* Finally, deliver header-fields via callback */
     for (size_t i = 0; i < PSEUDOHEADER_COUNT; ++i) {
         const struct aws_string *value_string = current_block->pseudoheader_values[i];
@@ -1256,6 +1289,21 @@ static struct aws_h2err s_process_header_field(
         goto malformed;
     }
 
+    if (!aws_strutil_is_http_field_value(header_field->value)) {
+        /**
+         * RFC9113 8.2.1  HTTP/2 implementations SHOULD validate field names and values, respectively, and treat
+         * messages that contain prohibited characters as malformed.
+         */
+        DECODER_LOG(ERROR, decoder, "Invalid header field, bad value");
+        DECODER_LOGF(
+            DEBUG,
+            decoder,
+            "Bad header field is: \"" PRInSTR ": " PRInSTR "\"",
+            AWS_BYTE_CURSOR_PRI(header_field->name),
+            AWS_BYTE_CURSOR_PRI(header_field->value));
+        goto malformed;
+    }
+
     enum aws_http_header_name name_enum = aws_http_lowercase_str_to_header_name(name);
 
     bool is_pseudoheader = name.ptr[0] == ':';
@@ -1329,8 +1377,6 @@ static struct aws_h2err s_process_header_field(
             }
         }
 
-        /* #TODO Validate characters used in header_field->value */
-
         switch (name_enum) {
             case AWS_HTTP_HEADER_COOKIE:
                 /* for a header cookie, we will not fire callback until we concatenate them all, let's store it at the
@@ -1363,7 +1409,18 @@ static struct aws_h2err s_process_header_field(
                     AWS_BYTE_CURSOR_PRI(name));
                 goto malformed;
             } break;
-
+            case AWS_HTTP_HEADER_TE: {
+                /* the TE header field, which MAY be present in an HTTP/2 request; when it is, it MUST NOT contain any
+                 * value other than "trailers" (RFC9113 8.2.2) */
+                if (!aws_byte_cursor_eq_c_str(&header_field->value, "trailers")) {
+                    DECODER_LOGF(
+                        ERROR,
+                        decoder,
+                        "TE header has value:'" PRInSTR "', not allowed in HTTP/2",
+                        AWS_BYTE_CURSOR_PRI(header_field->value));
+                    goto malformed;
+                }
+            } break;
             case AWS_HTTP_HEADER_CONTENT_LENGTH:
                 if (current_block->body_headers_forbidden) {
                     /* The content-length are forbidden */
@@ -1529,12 +1586,6 @@ static struct aws_h2err s_state_fn_header_block_entry(struct aws_h2_decoder *dec
 
     /* Finished decoding HPACK entry! */
 
-    /* #TODO Enforces dynamic table resize rules from RFC-7541 4.2
-     * If dynamic table size changed via SETTINGS frame, next header-block must start with DYNAMIC_TABLE_RESIZE entry.
-     * Is it illegal to receive a resize entry at other times? */
-
-    /* #TODO The TE header field ... MUST NOT contain any value other than "trailers" */
-
     if (result.type == AWS_HPACK_DECODE_T_HEADER_FIELD) {
         const struct aws_http_header *header_field = &result.data.header_field;
 

source/h2_frames.c

@@ -504,8 +504,6 @@ static struct aws_h2_frame *s_frame_new_headers_or_push_promise(
     const struct aws_h2_frame_priority_settings *optional_priority,
     uint32_t promised_stream_id) {
 
-    /* TODO: Host and ":authority" are no longer permitted to disagree. Should we enforce it here or sent it as
-     * requested, let the server side reject the request? */
     AWS_PRECONDITION(allocator);
     AWS_PRECONDITION(frame_type == AWS_H2_FRAME_T_HEADERS || frame_type == AWS_H2_FRAME_T_PUSH_PROMISE);
     AWS_PRECONDITION(headers);
@@ -1228,7 +1226,8 @@ int aws_h2_encode_frame(
 
 void aws_h2_frame_encoder_set_setting_header_table_size(struct aws_h2_frame_encoder *encoder, uint32_t data) {
     /* Setting for dynamic table size changed from peer, we will update the dynamic table size when we encoder the next
-     * header block */
+     * header block. The spec requires us to update the table if the updated size is smaller than the current size, but
+     * we can still update it even if not */
     aws_hpack_set_max_table_size(encoder->hpack, data);
     aws_hpack_set_protocol_max_size_setting(encoder->hpack, data);
 }

source/hpack.c

@@ -17,8 +17,6 @@
 
 /* #TODO split hpack encoder/decoder into different types */
 
-/* #TODO test empty strings */
-
 /* RFC-7540 6.5.2 */
 const size_t s_hpack_dynamic_table_initial_size = 4096;
 const size_t s_hpack_dynamic_table_initial_elements = 512;
@@ -240,7 +238,6 @@ struct aws_hpack_context {
         size_t size;
         size_t max_size;
 
-        /* TODO: check the new (RFC 9113 - 4.3.1) to make sure we did it right */
         /* SETTINGS_HEADER_TABLE_SIZE from http2 */
         size_t protocol_max_size_setting;
         /* aws_http_header * -> size_t */
@@ -458,7 +455,7 @@ const struct aws_http_header *aws_hpack_get_header(const struct aws_hpack_contex
 
 static const struct aws_http_header *s_get_header_u64(const struct aws_hpack_context *context, uint64_t index) {
     if (index > SIZE_MAX) {
-        HPACK_LOG(ERROR, context, "Header index is absurdly large")
+        HPACK_LOG(ERROR, context, "Header index is absurdly large");
         aws_raise_error(AWS_ERROR_INVALID_INDEX);
         return NULL;
     }
@@ -661,8 +658,16 @@ int aws_hpack_insert_header(struct aws_hpack_context *context, const struct aws_
 
     /* If for whatever reason this new header is bigger than the total table size, burn everything to the ground. */
     if (AWS_UNLIKELY(header_size > context->dynamic_table.max_size)) {
-        /* #TODO handle this. It's not an error. It should simply result in an empty table RFC-7541 4.4 */
-        goto error;
+        /* It's not an error. It should simply result in an empty table RFC-7541 4.4 */
+        HPACK_LOG(
+            WARN,
+            context,
+            "Attempt to add an entry larger than the maximum size causes the table to be emptied of all existing "
+            "entries and results in an empty table");
+        if (s_dynamic_table_shrink(context, 0)) {
+            goto error;
+        }
+        return AWS_OP_SUCCESS;
     }
 
     /* Rotate out headers until there's room for the new header (this function will return immediately if nothing needs
@@ -699,7 +704,6 @@ int aws_hpack_insert_header(struct aws_hpack_context *context, const struct aws_
     /* Put the header at the "front" of the table */
     struct aws_http_header *table_header = s_dynamic_table_get(context, 0);
 
-    /* TODO:: We can optimize this with ring buffer. */
     /* allocate memory for the name and value, which will be deallocated whenever the entry is evicted from the table or
      * the table is cleaned up. We keep the pointer in the name pointer of each entry */
     const size_t buf_memory_size = header->name.len + header->value.len;
@@ -1103,6 +1107,26 @@ int aws_hpack_decode(
                     context->progress_entry.u.literal.prefix_size = 4;
                     context->progress_entry.state = HPACK_ENTRY_STATE_LITERAL_BEGIN;
                 }
+
+                if (context->dynamic_table.protocol_max_size_setting < context->dynamic_table.size) {
+                    /**
+                     * RFC-9113 4.3.1 An endpoint MUST treat a field block that follows an acknowledgment of the
+                     * reduction to the maximum dynamic table size as a connection error of type
+                     * COMPRESSION_ERROR if it does not start with a conformant Dynamic Table Size Update instruction.
+                     *
+                     * The protocol max will only be updated once the SETTING ACK received.
+                     */
+                    if (context->progress_entry.state != HPACK_ENTRY_STATE_DYNAMIC_TABLE_RESIZE) {
+                        /* it will result in AWS_HTTP2_ERR_COMPRESSION_ERROR  */
+                        HPACK_LOG(
+                            ERROR,
+                            context,
+                            "SETTINGS_HEADER_TABLE_SIZE below the current size and other end has acknowledged the "
+                            "change, but not started with a conformant Dynamic Table Size Update instruction as "
+                            "required");
+                        return aws_raise_error(AWS_ERROR_INVALID_STATE);
+                    }
+                }
             } break;
 
             /* RFC-7541 6.1. Indexed Header Field Representation.
@@ -1239,7 +1263,7 @@ int aws_hpack_decode(
                 /* The new maximum size MUST be lower than or equal to the limit determined by the protocol using HPACK.
                  * A value that exceeds this limit MUST be treated as a decoding error. */
                 if (*size64 > context->dynamic_table.protocol_max_size_setting) {
-                    HPACK_LOG(ERROR, context, "Dynamic table update size is larger than the protocal setting");
+                    HPACK_LOG(ERROR, context, "Dynamic table update size is larger than the protocol setting");
                     return aws_raise_error(AWS_ERROR_INVALID_ARGUMENT);
                 }
                 size_t size = (size_t)*size64;

source/http2_stream_manager.c

@@ -817,9 +817,17 @@ static void s_make_request_task(struct aws_channel_task *task, void *arg, enum a
         (void *)pending_stream_acquisition,
         (void *)sm_connection->connection);
     bool is_shutting_down = false;
+    bool connection_new_requests_allowed = aws_http_connection_new_requests_allowed(sm_connection->connection);
     { /* BEGIN CRITICAL SECTION */
         s_lock_synced_data(stream_manager);
         is_shutting_down = stream_manager->synced_data.state != AWS_H2SMST_READY;
+        if (!is_shutting_down && !connection_new_requests_allowed) {
+            /* Push the pending stream acquisition back to the list instead of failing it. */
+            pending_stream_acquisition->sm_connection = NULL;
+            aws_linked_list_push_back(
+                &stream_manager->synced_data.pending_stream_acquisitions, &pending_stream_acquisition->node);
+            s_sm_count_increase_synced(stream_manager, AWS_SMCT_PENDING_ACQUISITION, 1);
+        }
         s_sm_count_decrease_synced(stream_manager, AWS_SMCT_PENDING_MAKE_REQUESTS, 1);
         /* The stream has not open yet, but we increase the count here, if anything fails, the count will be decreased
          */
@@ -849,6 +857,18 @@ static void s_make_request_task(struct aws_channel_task *task, void *arg, enum a
         error_code = AWS_ERROR_HTTP_STREAM_MANAGER_SHUTTING_DOWN;
         goto error;
     }
+    if (!connection_new_requests_allowed) {
+        STREAM_MANAGER_LOGF(
+            DEBUG,
+            stream_manager,
+            "acquisition:%p was assigned to be executed from connection:%p, however, connection is not available now, "
+            "put it back to the list waiting for it to be picked up by other connection.",
+            (void *)pending_stream_acquisition,
+            (void *)sm_connection->connection);
+        s_sm_connection_on_scheduled_stream_finishes(sm_connection, stream_manager);
+        return;
+    }
+
     struct aws_http_make_request_options request_options = {
         .self_size = sizeof(request_options),
         .request = pending_stream_acquisition->request,
@@ -858,8 +878,6 @@ static void s_make_request_task(struct aws_channel_task *task, void *arg, enum a
         .on_complete = s_on_stream_complete,
         .user_data = pending_stream_acquisition,
     };
-    /* TODO: we could put the pending acquisition back to the list if the connection is not available for new request.
-     */
 
     struct aws_http_stream *stream = aws_http_connection_make_request(sm_connection->connection, &request_options);
     if (!stream) {

source/request_response.c

@@ -141,8 +141,6 @@ int aws_http_headers_add_header(struct aws_http_headers *headers, const struct a
     bool front = false;
     if (pseudo && aws_http_headers_count(headers)) {
         struct aws_http_header last_header;
-        /* TODO: instead if checking the last header, maybe we can add the pseudo headers to the end of the existing
-         * pseudo headers, which needs to insert to the middle of the array list. */
         AWS_ZERO_STRUCT(last_header);
         aws_http_headers_get_index(headers, aws_http_headers_count(headers) - 1, &last_header);
         front = !aws_strutil_is_http_pseudo_header_name(last_header.name);

source/strutil.c

@@ -153,9 +153,10 @@ static const bool s_http_field_content_table[256] = {
 };
 
 /**
- * From RFC7230 section 3.2:
- * field-value    = *( field-content / obs-fold )
+ * From RFC9110 section 5.5:
+ * field-value    = *field-content
  * field-content  = field-vchar [ 1*( SP / HTAB ) field-vchar ]
+ * field-vchar    = VCHAR / obs-text
  *
  * But we're forbidding obs-fold
  */