[http] add llhttp parser implementation #15814

asraa · 2021-04-01T20:14:53Z

Signed-off-by: Asra Ali [email protected]

Commit Message: Add llhttp parser implementation.
Risk Level: High, but off by default.
Testing: H/1 codec impl and unit tests that use actual codecs have two targets now, a legacy and new version. Integration tests are parametrized by enabling the runtime flag via the config setting and is enabled in compile_time_options. Codec fuzz test enables the new parser.
Release Notes: Added
Runtime guard: envoy.reloadable_features.enable_new_http1_parser. Off by default.
Fixes: #5155
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=25777 (see note below)

CHANGES

We use llhttp to detect and reject invalid header characters. Thus, http1.invalid_characters response codes appear as http1.codec_error. and the status message reads HPE_INVALID_HEADER_TOKEN.
It also appears that llhttp handles invalid methods errors after message begin, whereas http-parser handles it at message begin. I only noticed this when it triggered a codec fuzzer failure when a second H/1 stream started. http-parser catches the invalid method failure and exits early, whereas llhttp hits the latent bug: there were no stream resets when responses were completed before requests are finished. In the normal case, we expect higher level code to reset the stream.

NOTES FOR CLEANUP

Our invalid character check in the codec can be removed when the legacy code is removed.
The secondary impl layer can be removed when the legacy code is removed. Actually, the parser interface should totally be removed since there's a slight performance dip from using it.

Co-authored-by: Derek Argueta <[email protected]> Signed-off-by: Asra Ali <[email protected]>

repokitteh-read-only · 2021-04-01T20:14:58Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #15814 was opened by asraa.

see: more, trace.

bazel/repository_locations.bzl

Signed-off-by: Asra Ali <[email protected]>

moderation · 2021-04-02T15:03:28Z

/lgtm deps

Signed-off-by: Asra Ali <[email protected]>

asraa · 2021-04-05T13:14:31Z

Very confused about the ASan error. It is reproducible via bazel test, but not by gdb or running the binary directly.

When I bazel test, it appears the function doesn't enter llhttp_execute at all but somehow returns from it causing undefined behavior using error after.

envoy/source/common/http/http1/parser_impl.cc

Lines 102 to 106 in 591811f

    
             error = llhttp_execute(&parser_, slice, len); 
        
           } 
        
           size_t nread = len; 
        
           // Adjust number of bytes read in case of error. 
        
           if (error != HPE_OK) {

Signed-off-by: Asra Ali <[email protected]>

antoniovicente

It is really exciting to see progress on Envoy moving to a http1 parser that is under active development.

source/common/http/http1/parser_impl.cc

test/integration/base_integration_test.cc

antoniovicente · 2021-04-05T23:45:36Z

source/common/http/http1/codec_impl.cc

@@ -766,6 +771,7 @@ StatusOr<ParserStatus> ConnectionImpl::onHeadersComplete() {
      return codecProtocolError("http/1.1 protocol error: unsupported transfer encoding");
    }
  }
+  parser_->hasContentLength(request_or_response_headers.ContentLength() != nullptr);


What happens if a request has both transfer-encoding: chunked and content-length?

https://github.com/nodejs/llhttp/blob/aa07189d39b8d0da9d5a887d93761014a45804bf/test/request/content-length.md#error-on-simultaneous-content-length-and-transfer-encoding-identity

It's handled with an error. Actually this is just because llhttp doesn't have an initial value for unset content length. http_parser uses all 1 bits set to indicate there is no content length. Added comment.

We should have tests in envoy to cover these cases. This is related to my question regarding use of lenient parsing further down in this file. It seems that those lenient options were added for the benefit of Envoy's H1 implementation.

envoy/test/common/http/http1/codec_impl_test.cc

Line 1945 in 56d3f8a

TEST_F(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength0) {

There are tests for both content length and chunked.

source/common/http/http1/parser_impl.cc

antoniovicente · 2021-04-05T23:58:17Z

source/common/http/http1/parser_impl.cc

+  void resume() { llhttp_resume(&parser_); }
+
+  ParserStatus pause() {
+    // llhttp can only pause by returning a paused status in user callbacks.


Should we do something to reconcile this behavior with http_parser? How do we ensure that nothing depends on this function having side effects for http_parser?

The http_parser implementation of pause() uses the http parser method to pause (https://github.com/nodejs/http-parser/blob/ec8b5ee63f0e51191ea43bb0c6eac7bfbff3141d/http_parser.h#L438) and an OK. That's the same as returning pause from llhttp parser. I think it's already reconciled

I guess this is an area to cleanup once llhttp is the only implementation.

In the current world I would consider something like:
ParserStatus maybePause(ParserStatus status);

The http_parser version would detect status == Paused and call its pause method and return Success, while the llhttp version would just return the status that was passed in.

In the current world I would consider something like:

To me that seems strange because Paused is the only status that maybePause would ever call.

Technically returning HPE_PAUSED from http-parser will also cause the parser to pause, but I don't trust making the change because there's very little http-parser docs and I don't know if it changes how it handles errors. (All http_parser_pause() does is cause HPE_PAUSED to be returning from execute. we could just return it directly).

source/common/http/http1/parser_impl.h

test/integration/base_integration_test.cc

bazel/repository_locations.bzl

Signed-off-by: Asra Ali <[email protected]>

indutny · 2021-04-06T16:13:13Z

I would appreciate if you could upgrade llhttp to 5.1.0 or 4.1.0, which was just released: nodejs/llhttp#95

bazel/repository_locations.bzl

Signed-off-by: Asra Ali <[email protected]>

asraa · 2021-04-11T18:49:35Z

Updated, sorry for the delay! (This is an evening thing right now)

For unit test dupes: only mixed_conn_pool test used an actual parser (I added an ASSERT(false) in the default parser and ran all the http/... conn_pool/... to grab failures).
A latent codec fuzz bug needed fixing, I noted why it came up in the PR description.
llhttp version's bumped.
Let me know what you think about the lenient chunked length setting. I switched over to llhttp's strict header validation since Envoy no longer provides the option to disable that.

Signed-off-by: Asra Ali <[email protected]>

asraa · 2021-04-12T00:21:19Z

Still working on the asan issue (comment above) and a tsan one. TSan is from running existing parser, reproducible, so something must have went wrong there... all the issues are about allowing chunked... and run into data race at destruction

antoniovicente · 2021-04-12T21:27:31Z

source/common/http/http1/parser_impl.cc

+public:
+  Impl(llhttp_type_t type) {
+    llhttp_init(&parser_, type, &settings_);
+    llhttp_set_lenient_chunked_length(&parser_, 1);


What code in Envoy is generating the error response when both transfer-encoding: chunked and content-length is set?

antoniovicente · 2021-04-12T21:29:13Z

.../http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5689430000795648

@@ -0,0 +1,62 @@
+actions {


What fixes are needed to make this corpus entry run successfully? I assume it's the changes to the fuzz harness?

It does run successfully now, with the changes in the diff that close that reset stream when the response ends before the request does.

test/common/http/codec_impl_fuzz_test.cc

Signed-off-by: Asra Ali <[email protected]>

antoniovicente · 2021-05-21T03:02:36Z

I tried to do some debugging but the conclusion I got to was along the lines that the stall happens even if I disable the cache filter. This may be a real issue processing multiple requests on the same connection with the new parser. Is there a chance that some state is not being reset in the parser between requests?

Debugging the state of the llhttp parser isn't easy between the typescript to c transcoding and it being an external dep for the build. Would it be possible to add methods to dump info about the state of the parser or debug accessors which can be used to implement said debug methods in ParserImpl?

moderation · 2021-06-14T18:53:32Z

llhttp 6.0.3 is out - https://github.com/nodejs/llhttp/releases/tag/release%2Fv6.0.3

Signed-off-by: Asra Ali <[email protected]>

antoniovicente · 2021-06-18T22:34:41Z

Please see precheck format presubmit error.

/wait

021-06-17T20:30:38.8262526Z ERROR: ./docs/root/version_history/current.rst:82: Version history not in alphabetical order (crash support vs admission control): please check placement of line
2021-06-17T20:30:38.8264348Z  * admission control: added :ref:`admission control <envoy_v3_api_field_extensions.filters.http.admission_control.v3alpha.AdmissionControl.rps_threshold>` option that when average RPS of the sampling window is below this threshold, the filter will not throttle requests. Added :ref:`admission control <envoy_v3_api_field_extensions.filters.http.admission_control.v3alpha.AdmissionControl.max_rejection_probability>` option to set an upper limit on the probability of rejection..

Signed-off-by: Asra Ali <[email protected]>

antoniovicente · 2021-06-21T19:03:28Z

cache filter integration test still failing

/wait

moderation · 2021-06-28T15:48:43Z

llhttp 6.0.4 is out - https://github.com/nodejs/llhttp/releases/tag/release%2Fv6.0.4

github-actions · 2021-07-28T16:01:19Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

moderation · 2021-08-27T14:53:15Z

llhttp 6.0.5 is out - https://github.com/nodejs/llhttp/releases/tag/release%2Fv6.0.5

yanavlasov · 2022-10-28T20:36:52Z

Obsolete. QUICHE H/1 code (Balsa) is currently being integrated.

add llhttp

1e9d3be

Co-authored-by: Derek Argueta <[email protected]> Signed-off-by: Asra Ali <[email protected]>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Apr 1, 2021

moderation reviewed Apr 1, 2021

View reviewed changes

bazel/repository_locations.bzl Outdated Show resolved Hide resolved

add integration test support

204e519

Signed-off-by: Asra Ali <[email protected]>

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Apr 2, 2021

fix deps script weird URL

d458ac4

Signed-off-by: Asra Ali <[email protected]>

asraa changed the title ~~WIP add llhttp parser implementation~~ [http] add llhttp parser implementation Apr 2, 2021

mattklein123 assigned antoniovicente Apr 2, 2021

bump llhttp

591811f

Signed-off-by: Asra Ali <[email protected]>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Apr 5, 2021

antoniovicente reviewed Apr 6, 2021

View reviewed changes

antoniovicente added the waiting label Apr 6, 2021

fix initial comments

9398f11

Signed-off-by: Asra Ali <[email protected]>

repokitteh-read-only bot removed the waiting label Apr 6, 2021

antoniovicente reviewed Apr 7, 2021

View reviewed changes

bazel/repository_locations.bzl Outdated Show resolved Hide resolved

antoniovicente added the waiting label Apr 7, 2021

asraa added 2 commits April 10, 2021 17:32

Merge remote-tracking branch 'upstream/main' into add-llhttp

e4f6b56

Signed-off-by: Asra Ali <[email protected]>

make changes

397d027

Signed-off-by: Asra Ali <[email protected]>

repokitteh-read-only bot removed the waiting label Apr 11, 2021

fix release date once again

f342c0e

Signed-off-by: Asra Ali <[email protected]>

antoniovicente reviewed Apr 12, 2021

View reviewed changes

antoniovicente added the waiting:any label Apr 12, 2021

repokitteh-read-only bot removed the waiting:any label Apr 13, 2021

explicitly test both runtime vals

53232a4

Signed-off-by: Asra Ali <[email protected]>

repokitteh-read-only bot removed the waiting label May 20, 2021

antoniovicente added the waiting label May 21, 2021

derekargueta mentioned this pull request May 21, 2021

Replace parser C-extension with pure Ruby puma/puma#1889

Open

antoniovicente assigned yanavlasov and htuch May 27, 2021

asraa added 2 commits June 17, 2021 16:18

Merge remote-tracking branch 'upstream/main' into add-llhttp

c77400d

Signed-off-by: Asra Ali <[email protected]>

bump to 6.0.3

8f84cf4

Signed-off-by: Asra Ali <[email protected]>

repokitteh-read-only bot removed the waiting label Jun 17, 2021

repokitteh-read-only bot added the waiting label Jun 18, 2021

fix bad doc merge

7e480fe

Signed-off-by: Asra Ali <[email protected]>

repokitteh-read-only bot removed the waiting label Jun 21, 2021

fix codec fuzzer merge

2a41f11

Signed-off-by: Asra Ali <[email protected]>

repokitteh-read-only bot added the waiting label Jun 21, 2021

sschepens mentioned this pull request Jul 1, 2021

HPE_INVALID_URL with non-urlencoded args #12073

Open

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 28, 2021

yanavlasov added no stalebot Disables stalebot from closing an issue and removed stale stalebot believes this issue/PR has not been touched recently labels Jul 29, 2021

antoniovicente removed their assignment Aug 9, 2021

moderation mentioned this pull request Sep 10, 2021

com_github_fmtlib_fmt has a newer release 8.0.1 #18052

Closed

moderation mentioned this pull request Oct 22, 2021

cvescan: Exclude fails by version #18731

Merged

yanavlasov closed this Oct 28, 2022

zhxie mentioned this pull request Aug 23, 2023

Performance issue with Balsa Parser #29203

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[http] add llhttp parser implementation #15814

[http] add llhttp parser implementation #15814

asraa commented Apr 1, 2021 •

edited

Loading

repokitteh-read-only bot commented Apr 1, 2021

moderation commented Apr 2, 2021

asraa commented Apr 5, 2021 •

edited

Loading

antoniovicente left a comment

antoniovicente Apr 5, 2021

asraa Apr 6, 2021

antoniovicente Apr 7, 2021

asraa Apr 10, 2021

antoniovicente Apr 5, 2021

asraa Apr 6, 2021

antoniovicente Apr 26, 2021

asraa Apr 28, 2021

indutny commented Apr 6, 2021

asraa commented Apr 11, 2021

asraa commented Apr 12, 2021 •

edited

Loading

antoniovicente Apr 12, 2021

antoniovicente Apr 12, 2021

asraa Apr 13, 2021

antoniovicente commented May 21, 2021

moderation commented Jun 14, 2021

antoniovicente commented Jun 18, 2021

antoniovicente commented Jun 21, 2021

moderation commented Jun 28, 2021

github-actions bot commented Jul 28, 2021

moderation commented Aug 27, 2021

yanavlasov commented Oct 28, 2022

[http] add llhttp parser implementation #15814

[http] add llhttp parser implementation #15814

Conversation

asraa commented Apr 1, 2021 • edited Loading

repokitteh-read-only bot commented Apr 1, 2021

moderation commented Apr 2, 2021

asraa commented Apr 5, 2021 • edited Loading

antoniovicente left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented Apr 6, 2021

asraa commented Apr 11, 2021

asraa commented Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoniovicente commented May 21, 2021

moderation commented Jun 14, 2021

antoniovicente commented Jun 18, 2021

antoniovicente commented Jun 21, 2021

moderation commented Jun 28, 2021

github-actions bot commented Jul 28, 2021

moderation commented Aug 27, 2021

yanavlasov commented Oct 28, 2022

asraa commented Apr 1, 2021 •

edited

Loading

asraa commented Apr 5, 2021 •

edited

Loading

asraa commented Apr 12, 2021 •

edited

Loading