Address TODOs #383

TingDaoK · 2022-07-14T00:07:52Z

Address several TODOs in HTTP/2 code.
I think: it fixes the occasionally failure from osx local host built by add the connection monitor time to 2 secs(?)
Use assert_equal instead of assert_true, which will tell us what the number is on failure.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

source/h2_connection.c

source/h2_decoder.c

graebm · 2022-07-20T23:48:07Z

source/h2_decoder.c

+        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.");


do we need to check that these things aren't empty strings?

I see you checking the request_string, but not the others

I'm still wondering about this: should we be doing more validation on the pseudo-headers?
Like, are any of them allowed to be ""?

It was checked before. from the s_process_header_field at here

source/h2_decoder.c

graebm · 2022-07-22T00:25:19Z

source/h2_frames.c

 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);


I just spent like an hour trying to make sense of whether or not we're doing the right thing with this hpack resize stuff. MAN this stuff is hard to understand. We just have a lot of variables with similar names, and the 1 hpack class that has both encoding and decoding logic. Can I make some suggestions?

Don't have aws_h2_frame_encoder_set_setting_header_table_size() call aws_hpack_set_protocol_max_size_setting() at all. Turns out that function has no effect on an aws_hpack_context that is being used for encoding.

rename ~~aws_hpack_set_max_table_size()~~ ->

/* Call this after receiving SETTINGS_HEADER_TABLE_SIZE from peer and sending the ACK. * The hpack-encoder remembers all size updates, and makes sure to encode the proper * number of Dynamic Table Size Updates the next time a header block is sent. */ AWS_HTTP_API void aws_hpack_encoder_update_max_table_size(struct aws_hpack_context *context, uint32_t new_max_size);

rename ~~aws_hpack_set_protocol_max_size_setting()~~ ->

/* Call this after sending SETTINGS_HEADER_TABLE_SIZE and receiving ACK from the peer. * The hpack-decoder remembers all size updates, and makes sure that the peer * sends the appropriate Dynamic Table Size Updates in the next header block we receive. */ AWS_HTTP_API void aws_hpack_decoder_update_max_table_size(struct aws_hpack_context *context, uint32_t new_max_size);

I still don't think aws_hpack_set_protocol_max_size_setting() aka aws_hpack_decoder_update_max_table_size() is doing the right thing. In the same way the encoder tracks a smallest and last size, I think we need to do similar tracking for the decoder so we can properly check that the peer sends the right number of updates?

man, this is why we need to split ~~aws_hpack_context~~ into aws_hpack_encoder and aws_hpack_decoder. It's too confusing having everyone's variables and functions all mixed up together

graebm · 2022-08-22T21:37:36Z

include/aws/http/http.h

@@ -54,6 +54,7 @@ enum aws_http_errors {
    AWS_ERROR_HTTP_STREAM_MANAGER_SHUTTING_DOWN,
    AWS_ERROR_HTTP_STREAM_MANAGER_CONNECTION_ACQUIRE_FAILURE,
    AWS_ERROR_HTTP_STREAM_MANAGER_UNEXPECTED_HTTP_VERSION,
+    AWS_ERROR_HTTP_REQUIRED_PSEUDO_HEADER_MISSING,


I don't see this error used anywhere.
Remove it if you're not using it

it was used here

graebm · 2022-08-22T22:03:23Z

source/h2_decoder.c

+        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.");


I'm still wondering about this: should we be doing more validation on the pseudo-headers?
Like, are any of them allowed to be ""?

source/h2_decoder.c

graebm · 2022-08-22T22:18:43Z

source/h2_decoder.c

@@ -1253,6 +1288,21 @@ static struct aws_h2err s_process_header_field(
        goto malformed;
    }

+    if (!aws_strutil_is_http_field_value(header_field->value)) {


I just found a new TODO to add to our code. From RFC-9110:

A field value does not include leading or trailing whitespace. When a specific version of HTTP allows such whitespace to appear in a message, a field parsing implementation MUST exclude such whitespace prior to evaluating the field value

I see the H1 encoder and decoder using aws_strutil_trim_http_whitespace(), but I don't see us doing it anywhere in our H2 code

When a specific version of HTTP allows such whitespace to appear in a message

A field value MUST NOT start or end with an ASCII whitespace character (ASCII SP or HTAB, 0x20 or 0x09). here. So, I think we should not trim the white space for h2

source/h2_connection.c

source/hpack.c

source/hpack_decoder.c

graebm · 2022-08-22T23:13:02Z

source/hpack_decoder.c

                    decoder->progress_entry.u.literal.prefix_size = 4;
                    decoder->progress_entry.state = HPACK_ENTRY_STATE_LITERAL_BEGIN;
                }
+                if (decoder->dynamic_table_protocol_max_size_setting < decoder->context.dynamic_table.size) {


I don't think the logic here is correct

The encoder has the correct logic. In the dynamic_table_size_update struct it tracks latest_value and smallest_value separately.

struct { size_t latest_value; size_t smallest_value; bool pending; } dynamic_table_size_update;

The decoder needs to do something similar, if multiple SETTINGS packets are received back to back, and change the table size to 10, then to 100, then the decoder needs to confirm that it receives an update <= 10 (RFC-7541 4.2)

Hmmm, I read again, and still feel confusing about:

An endpoint MUST treat a field block that follows an acknowledgment of the reduction to the maximum dynamic table size as a connection error (Section 5.4.1) of type COMPRESSION_ERROR if it does not start with a conformant Dynamic Table Size Update instruction.

here should be the requirement from the decoder. Especially about start with a conformant Dynamic Table Size Update instruction. What is conformant update instruction for the case you pointing out to start with? I don't think there is any clear statement about what kind of Dynamic Table Size Update instruction to start with.

from hpack spec here:

the smallest maximum table size that occurs in that interval MUST be signaled in a dynamic table
size update. The final maximum size is always signaled, resulting in at most two dynamic table size updates

I guess the first one could be larger than the smallest value as long as the second one is smaller than it. :(

resulting in at most two dynamic table size updates

However, I don't see any place that enforce the dynamic table size updates to be no more than one in normal case. But I guess they assume that's the case as from here

In HTTP/2, this limit is the last value of the SETTINGS_HEADER_TABLE_SIZE parameter

Here is my understanding:

We can receive at most two continuous Dynamic Table Size Update instruction after ack the settings that could result in reduce the size.

One of those two MUST be smaller than the smallest max.

Everyone MUST be smaller than the latest max.

TingDaoK added 7 commits July 13, 2022 17:07

Address TODOs

355e6d8

TODOs in decoder

875f015

more tests

5733ac0

use int equal

e5ab40d

the java binding requires the time to be at least 2

a4fbcbb

fix the echo error if content length is set

f42c4b8

end stream if no data was present

b80322f

graebm reviewed Jul 20, 2022

View reviewed changes

graebm reviewed Jul 22, 2022

View reviewed changes

TingDaoK added 6 commits August 1, 2022 14:48

more uint equal

ee52c6a

Merge branch 'main' into adrress-todos

85f362f

allocate cannot fail now

b63cc70

Merge branch 'main' into adrress-todos

79b78d9

address comments

4beeba2

Merge branch 'main' into adrress-todos

9275fff

graebm reviewed Aug 22, 2022

View reviewed changes

TingDaoK added 6 commits September 8, 2022 13:24

non body server

f98f134

comment addressed

7b4bc8b

Merge branch 'main' into adrress-todos

cce49e2

rename the outdated variable name

68dcf9b

Merge branch 'main' into adrress-todos

052660c

Merge branch 'main' into adrress-todos

480cd2d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address TODOs #383

Address TODOs #383

TingDaoK commented Jul 14, 2022 •

edited

Loading

graebm Jul 20, 2022

graebm Aug 22, 2022

TingDaoK Sep 15, 2022

graebm Jul 22, 2022

graebm Aug 22, 2022

TingDaoK Sep 14, 2022

graebm Aug 22, 2022

graebm Aug 22, 2022

TingDaoK Sep 15, 2022 •

edited

Loading

graebm Aug 22, 2022

TingDaoK Sep 15, 2022 •

edited

Loading

TingDaoK Sep 15, 2022 •

edited

Loading

Address TODOs #383

Are you sure you want to change the base?

Address TODOs #383

Conversation

TingDaoK commented Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TingDaoK Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TingDaoK Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

TingDaoK Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

TingDaoK commented Jul 14, 2022 •

edited

Loading

TingDaoK Sep 15, 2022 •

edited

Loading

TingDaoK Sep 15, 2022 •

edited

Loading

TingDaoK Sep 15, 2022 •

edited

Loading