Skip to content

http2: remove exceptions from H/2 codec #11575

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yanavlasov merged 17 commits into from
Jul 31, 2020
Merged

http2: remove exceptions from H/2 codec #11575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
f4e28cd
H/2 codec: remove FrameFloodException
yanavlasov Jun 11, 2020
08d90c4
Remove trailers without end stream error
yanavlasov Jun 11, 2020
bdeccf1
Remove exception from sendPendingFrames
yanavlasov Jun 11, 2020
713f094
Remove exception from the innerDispatch
yanavlasov Jun 11, 2020
07cfb26
Cleanup
yanavlasov Jun 12, 2020
8c96d58
Cleanup
yanavlasov Jun 12, 2020
b05fb3c
Address comments
yanavlasov Jun 19, 2020
b54c35c
Address comments
yanavlasov Jun 19, 2020
b91be2a
Merge branch 'master' into exceptions-1
yanavlasov Jul 20, 2020
a2f868e
Fix tests
yanavlasov Jul 21, 2020
19039bc
Address comments
yanavlasov Jul 22, 2020
a0a4192
Merge branch 'master' into exceptions-1
yanavlasov Jul 22, 2020
7ac6e33
Use RETURN_IF_ERROR
yanavlasov Jul 24, 2020
74bf197
Replace RELEASE_ASSERT with ENVOY_BUG
yanavlasov Jul 24, 2020
9d26d43
Revert "Replace RELEASE_ASSERT with ENVOY_BUG"
yanavlasov Jul 28, 2020
8b9e5e7
Merge branch 'master' into exceptions-1
yanavlasov Jul 28, 2020
a252cd2
Merge branch 'master' into exceptions-1
yanavlasov Jul 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion source/common/http/http2/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ envoy_cc_library(
"abseil_inlined_vector",
"abseil_algorithm",
],
deps = CODEC_LIB_DEPS,
deps = CODEC_LIB_DEPS + [
"//source/common/common:statusor_lib",
],
)

envoy_cc_library(
Expand Down
218 changes: 129 additions & 89 deletions source/common/http/http2/codec_impl.cc
View file
Open in desktop

Large diffs are not rendered by default.

58 changes: 35 additions & 23 deletions source/common/http/http2/codec_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "common/buffer/watermark_buffer.h"
#include "common/common/linked_object.h"
#include "common/common/logger.h"
#include "common/common/statusor.h"
#include "common/common/thread.h"
#include "common/http/codec_helper.h"
#include "common/http/header_map_impl.h"
Expand Down Expand Up @@ -185,7 +186,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log

StreamImpl* base() { return this; }
ssize_t onDataSourceRead(uint64_t length, uint32_t* data_flags);
int onDataSourceSend(const uint8_t* framehd, size_t length);
Status onDataSourceSend(const uint8_t* framehd, size_t length);
void resetStreamWorker(StreamResetReason reason);
static void buildHeaders(std::vector<nghttp2_nv>& final_headers, const HeaderMap& headers);
void saveHeader(HeaderString&& name, HeaderString&& value);
Expand Down Expand Up @@ -391,7 +392,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
// that is not associated with an existing stream.
StreamImpl* getStream(int32_t stream_id);
int saveHeader(const nghttp2_frame* frame, HeaderString&& name, HeaderString&& value);
void sendPendingFrames();
Status sendPendingFrames();
void sendSettings(const envoy::config::core::v3::Http2ProtocolOptions& http2_options,
bool disable_push);
// Callback triggered when the peer's SETTINGS frame is received.
Expand All @@ -410,6 +411,12 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
return absl::nullopt;
}

/**
* Save `status` into nghttp2_callback_status_.
* Return nghttp2 callback return code corresponding to `status`.
*/
int setAndCheckNghttp2CallbackStatus(Status&& status);

static Http2Callbacks http2_callbacks_;

std::list<StreamImplPtr> active_streams_;
Expand All @@ -421,7 +428,12 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
uint32_t per_stream_buffer_limit_;
bool allow_metadata_;
const bool stream_error_on_invalid_http_messaging_;
bool flood_detected_;

// Status for any errors encountered by the nghttp2 callbacks.
// nghttp2 library uses single return code to indicate callback failure and
// `nghttp2_callback_status_` is used to save right error information returned by a callback. The
// `nghttp2_callback_status_` is valid iff nghttp call returned NGHTTP2_ERR_CALLBACK_FAILURE.
Status nghttp2_callback_status_;

// Set if the type of frame that is about to be sent is PING or SETTINGS with the ACK flag set, or
// RST_STREAM.
Expand Down Expand Up @@ -483,14 +495,14 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
// Http2FloodMitigationTest.* tests in test/integration/http2_integration_test.cc will break if
// this changes in the future. Also it is important that onSend does not do partial writes, as the
// nghttp2 library will keep calling this callback to write the rest of the frame.
ssize_t onSend(const uint8_t* data, size_t length);
StatusOr<ssize_t> onSend(const uint8_t* data, size_t length);

private:
virtual ConnectionCallbacks& callbacks() PURE;
virtual int onBeginHeaders(const nghttp2_frame* frame) PURE;
virtual Status onBeginHeaders(const nghttp2_frame* frame) PURE;
int onData(int32_t stream_id, const uint8_t* data, size_t len);
int onBeforeFrameReceived(const nghttp2_frame_hd* hd);
int onFrameReceived(const nghttp2_frame* frame);
Status onBeforeFrameReceived(const nghttp2_frame_hd* hd);
Status onFrameReceived(const nghttp2_frame* frame);
int onBeforeFrameSend(const nghttp2_frame* frame);
int onFrameSend(const nghttp2_frame* frame);
int onError(absl::string_view error);
Expand All @@ -501,12 +513,12 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
int onMetadataFrameComplete(int32_t stream_id, bool end_metadata);
ssize_t packMetadata(int32_t stream_id, uint8_t* buf, size_t len);
// Adds buffer fragment for a new outbound frame to the supplied Buffer::OwnedImpl.
// Returns true on success or false if outbound queue limits were exceeded.
bool addOutboundFrameFragment(Buffer::OwnedImpl& output, const uint8_t* data, size_t length);
virtual void checkOutboundQueueLimits() PURE;
void incrementOutboundFrameCount(bool is_outbound_flood_monitored_control_frame);
virtual bool trackInboundFrames(const nghttp2_frame_hd* hd, uint32_t padding_length) PURE;
virtual bool checkInboundFrameLimits(int32_t stream_id) PURE;
// Returns Ok Status on success or error if outbound queue limits were exceeded.
Status addOutboundFrameFragment(Buffer::OwnedImpl& output, const uint8_t* data, size_t length);
virtual Status checkOutboundQueueLimits() PURE;
Status incrementOutboundFrameCount(bool is_outbound_flood_monitored_control_frame);
Copy link
Contributor

@alyssawilk alyssawilk Jul 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we think of ABSL_MUST_USE_RESULT for these to make it clear (and compiler checked ) we're doing the right thing?

Copy link
Contributor Author

@yanavlasov yanavlasov Jul 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole Status class is marked like this here, so any time a method returns Status it is marked that it must be checked.

virtual Status trackInboundFrames(const nghttp2_frame_hd* hd, uint32_t padding_length) PURE;
virtual Status checkInboundFrameLimits(int32_t stream_id) PURE;
void releaseOutboundFrame();
void releaseOutboundControlFrame();

Expand Down Expand Up @@ -534,19 +546,19 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
private:
// ConnectionImpl
ConnectionCallbacks& callbacks() override { return callbacks_; }
int onBeginHeaders(const nghttp2_frame* frame) override;
Status onBeginHeaders(const nghttp2_frame* frame) override;
int onHeader(const nghttp2_frame* frame, HeaderString&& name, HeaderString&& value) override;

// Presently client connections only perform accounting of outbound frames and do not
// terminate connections when queue limits are exceeded. The primary reason is the complexity of
// the clean-up of upstream connections. The clean-up of upstream connection causes RST_STREAM
// messages to be sent on corresponding downstream connections. This may actually trigger flood
// mitigation on the downstream connections, which causes an exception to be thrown in the middle
// of the clean-up loop, leaving resources in a half cleaned up state.
// mitigation on the downstream connections, however there is currently no mechanism for
// handling these types of errors.
// TODO(yanavlasov): add flood mitigation for upstream connections as well.
void checkOutboundQueueLimits() override {}
bool trackInboundFrames(const nghttp2_frame_hd*, uint32_t) override { return true; }
bool checkInboundFrameLimits(int32_t) override { return true; }
Status checkOutboundQueueLimits() override { return okStatus(); }
Status trackInboundFrames(const nghttp2_frame_hd*, uint32_t) override { return okStatus(); }
Status checkInboundFrameLimits(int32_t) override { return okStatus(); }

Http::ConnectionCallbacks& callbacks_;
};
Expand All @@ -567,11 +579,11 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
private:
// ConnectionImpl
ConnectionCallbacks& callbacks() override { return callbacks_; }
int onBeginHeaders(const nghttp2_frame* frame) override;
Status onBeginHeaders(const nghttp2_frame* frame) override;
int onHeader(const nghttp2_frame* frame, HeaderString&& name, HeaderString&& value) override;
void checkOutboundQueueLimits() override;
bool trackInboundFrames(const nghttp2_frame_hd* hd, uint32_t padding_length) override;
bool checkInboundFrameLimits(int32_t stream_id) override;
Status checkOutboundQueueLimits() override;
Status trackInboundFrames(const nghttp2_frame_hd* hd, uint32_t padding_length) override;
Status checkInboundFrameLimits(int32_t stream_id) override;
absl::optional<int> checkHeaderNameForUnderscores(absl::string_view header_name) override;

// Http::Connection
Expand Down
16 changes: 12 additions & 4 deletions test/common/http/http2/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ envoy_cc_test_library(
"//source/common/http:utility_lib",
"//source/common/http/http2:codec_lib",
"//test/common/http:common_lib",
"//test/common/http/http2:codec_impl_test_util",
"//test/mocks/http:http_mocks",
"//test/mocks/network:network_mocks",
"//test/test_common:environment_lib",
Expand All @@ -124,7 +123,10 @@ envoy_cc_test(
"response_header_corpus/simple_example_plain",
],
tags = ["fails_on_windows"],
deps = [":frame_replay_lib"],
deps = [
":frame_replay_lib",
"//test/common/http/http2:codec_impl_test_util",
],
)

envoy_cc_test(
Expand All @@ -147,12 +149,18 @@ envoy_cc_fuzz_test(
name = "response_header_fuzz_test",
srcs = ["response_header_fuzz_test.cc"],
corpus = "response_header_corpus",
deps = [":frame_replay_lib"],
deps = [
":frame_replay_lib",
"//test/common/http/http2:codec_impl_test_util",
],
)

envoy_cc_fuzz_test(
name = "request_header_fuzz_test",
srcs = ["request_header_fuzz_test.cc"],
corpus = "request_header_corpus",
deps = [":frame_replay_lib"],
deps = [
":frame_replay_lib",
"//test/common/http/http2:codec_impl_test_util",
],
)
Loading