From b0be102a29ab325ec6f6694078ceacbd8a3745a6 Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Mon, 10 Aug 2020 13:03:34 -0700 Subject: [PATCH 1/9] Fixes stack overflow in http inspector test Signed-off-by: Sotiris Nanopoulos --- .../filters/listener/http_inspector/BUILD | 3 +++ .../http_inspector/http_inspector_test.cc | 27 ++++++++++--------- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/test/extensions/filters/listener/http_inspector/BUILD b/test/extensions/filters/listener/http_inspector/BUILD index 8530e24434d9..6735db161696 100644 --- a/test/extensions/filters/listener/http_inspector/BUILD +++ b/test/extensions/filters/listener/http_inspector/BUILD @@ -15,6 +15,9 @@ envoy_extension_cc_test( name = "http_inspector_test", srcs = ["http_inspector_test.cc"], extension_name = "envoy.filters.listener.http_inspector", + #TODO(davinci26): The test passes on Windows *but* http inspector + # relies on Event::FileTriggerType::Edge and we get away with it + # because we mock the dispather. tags = ["fails_on_windows"], deps = [ "//source/common/common:hex_lib", diff --git a/test/extensions/filters/listener/http_inspector/http_inspector_test.cc b/test/extensions/filters/listener/http_inspector/http_inspector_test.cc index a6638892f26f..f42ac610b26e 100644 --- a/test/extensions/filters/listener/http_inspector/http_inspector_test.cc +++ b/test/extensions/filters/listener/http_inspector/http_inspector_test.cc @@ -584,19 +584,20 @@ TEST_F(HttpInspectorTest, Http1WithLargeRequestLine) { num_loops = 2; #endif - for (size_t i = 1; i <= num_loops; i++) { - size_t len = i; - if (num_loops == 2) { - len = size_t(Config::MAX_INSPECT_SIZE / (3 - i)); - } - EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) - .WillOnce(Invoke( - [&data, len](os_fd_t, void* buffer, size_t length, int) -> Api::SysCallSizeResult { - ASSERT(length >= len); - memcpy(buffer, data.data(), len); - return Api::SysCallSizeResult{ssize_t(len), 0}; - })); - } + size_t ctr = 1; + EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) + .Times(num_loops) + .WillRepeatedly(Invoke( + [&data, &ctr, num_loops](os_fd_t, void* buffer, size_t length, int) -> Api::SysCallSizeResult { + size_t len = ctr; + if (num_loops == 2) { + len = size_t(Config::MAX_INSPECT_SIZE / (3 - ctr)); + } + ASSERT(length >= len); + memcpy(buffer, data.data(), len); + ctr += 1; + return Api::SysCallSizeResult{ssize_t(len), 0}; + })); } bool got_continue = false; From dec3ba974521c71d581e24c7591500a69b831db7 Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Mon, 10 Aug 2020 13:54:32 -0700 Subject: [PATCH 2/9] fixes format Signed-off-by: Sotiris Nanopoulos --- .../listener/http_inspector/http_inspector_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/extensions/filters/listener/http_inspector/http_inspector_test.cc b/test/extensions/filters/listener/http_inspector/http_inspector_test.cc index f42ac610b26e..215418811b7d 100644 --- a/test/extensions/filters/listener/http_inspector/http_inspector_test.cc +++ b/test/extensions/filters/listener/http_inspector/http_inspector_test.cc @@ -586,9 +586,9 @@ TEST_F(HttpInspectorTest, Http1WithLargeRequestLine) { size_t ctr = 1; EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) - .Times(num_loops) - .WillRepeatedly(Invoke( - [&data, &ctr, num_loops](os_fd_t, void* buffer, size_t length, int) -> Api::SysCallSizeResult { + .Times(num_loops) + .WillRepeatedly(Invoke([&data, &ctr, num_loops](os_fd_t, void* buffer, size_t length, + int) -> Api::SysCallSizeResult { size_t len = ctr; if (num_loops == 2) { len = size_t(Config::MAX_INSPECT_SIZE / (3 - ctr)); @@ -597,7 +597,7 @@ TEST_F(HttpInspectorTest, Http1WithLargeRequestLine) { memcpy(buffer, data.data(), len); ctr += 1; return Api::SysCallSizeResult{ssize_t(len), 0}; - })); + })); } bool got_continue = false; From 76833c8cd8cd359dbf2142d14638d93df770dc5e Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Thu, 13 Aug 2020 17:34:13 -0700 Subject: [PATCH 3/9] Fixes ASAN Signed-off-by: Sotiris Nanopoulos --- .../listener/http_inspector/http_inspector_test.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/extensions/filters/listener/http_inspector/http_inspector_test.cc b/test/extensions/filters/listener/http_inspector/http_inspector_test.cc index 215418811b7d..e6229137cb2b 100644 --- a/test/extensions/filters/listener/http_inspector/http_inspector_test.cc +++ b/test/extensions/filters/listener/http_inspector/http_inspector_test.cc @@ -584,18 +584,18 @@ TEST_F(HttpInspectorTest, Http1WithLargeRequestLine) { num_loops = 2; #endif - size_t ctr = 1; + auto ctr = std::make_shared(1); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) .Times(num_loops) - .WillRepeatedly(Invoke([&data, &ctr, num_loops](os_fd_t, void* buffer, size_t length, + .WillRepeatedly(Invoke([&data, ctr, num_loops](os_fd_t, void* buffer, size_t length, int) -> Api::SysCallSizeResult { - size_t len = ctr; + size_t len = (*ctr); if (num_loops == 2) { - len = size_t(Config::MAX_INSPECT_SIZE / (3 - ctr)); + len = size_t(Config::MAX_INSPECT_SIZE / (3 - (*ctr))); } ASSERT(length >= len); memcpy(buffer, data.data(), len); - ctr += 1; + (*ctr) += 1; return Api::SysCallSizeResult{ssize_t(len), 0}; })); } From 212418a7bca2c225cf8e11b165c82a3817e93578 Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Thu, 13 Aug 2020 18:01:15 -0700 Subject: [PATCH 4/9] ws Signed-off-by: Sotiris Nanopoulos --- .../filters/listener/http_inspector/http_inspector_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/listener/http_inspector/http_inspector_test.cc b/test/extensions/filters/listener/http_inspector/http_inspector_test.cc index e6229137cb2b..1caa7e501bc6 100644 --- a/test/extensions/filters/listener/http_inspector/http_inspector_test.cc +++ b/test/extensions/filters/listener/http_inspector/http_inspector_test.cc @@ -588,7 +588,7 @@ TEST_F(HttpInspectorTest, Http1WithLargeRequestLine) { EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) .Times(num_loops) .WillRepeatedly(Invoke([&data, ctr, num_loops](os_fd_t, void* buffer, size_t length, - int) -> Api::SysCallSizeResult { + int) -> Api::SysCallSizeResult { size_t len = (*ctr); if (num_loops == 2) { len = size_t(Config::MAX_INSPECT_SIZE / (3 - (*ctr))); From d540ffd472c7a3ceb9097f1b09b1d16acbf8b4bc Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Fri, 14 Aug 2020 09:57:16 -0700 Subject: [PATCH 5/9] Address PR comments Signed-off-by: Sotiris Nanopoulos --- test/extensions/filters/listener/http_inspector/BUILD | 5 +++-- .../filters/listener/http_inspector/http_inspector_test.cc | 6 +++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/test/extensions/filters/listener/http_inspector/BUILD b/test/extensions/filters/listener/http_inspector/BUILD index 6735db161696..d4c49021da09 100644 --- a/test/extensions/filters/listener/http_inspector/BUILD +++ b/test/extensions/filters/listener/http_inspector/BUILD @@ -16,8 +16,9 @@ envoy_extension_cc_test( srcs = ["http_inspector_test.cc"], extension_name = "envoy.filters.listener.http_inspector", #TODO(davinci26): The test passes on Windows *but* http inspector - # relies on Event::FileTriggerType::Edge and we get away with it - # because we mock the dispather. + # *used* to rely on Event::FileTriggerType::Edge and we got away with it + # because we mock the dispatcher. Need to verify that the scenario is + # actually working. tags = ["fails_on_windows"], deps = [ "//source/common/common:hex_lib", diff --git a/test/extensions/filters/listener/http_inspector/http_inspector_test.cc b/test/extensions/filters/listener/http_inspector/http_inspector_test.cc index 1caa7e501bc6..19e909ec6a77 100644 --- a/test/extensions/filters/listener/http_inspector/http_inspector_test.cc +++ b/test/extensions/filters/listener/http_inspector/http_inspector_test.cc @@ -567,6 +567,9 @@ TEST_F(HttpInspectorTest, MultipleReadsHttp1BadProtocol) { } TEST_F(HttpInspectorTest, Http1WithLargeRequestLine) { + // Verify that the http inspector can detect http requests + // with large request line even when they are splitted over + // multiple recv calls. init(); absl::string_view method = "GET", http = "/index HTTP/1.0\r"; std::string spaces(Config::MAX_INSPECT_SIZE - method.size() - http.size(), ' '); @@ -591,11 +594,12 @@ TEST_F(HttpInspectorTest, Http1WithLargeRequestLine) { int) -> Api::SysCallSizeResult { size_t len = (*ctr); if (num_loops == 2) { + ASSERT(*ctx != 3) len = size_t(Config::MAX_INSPECT_SIZE / (3 - (*ctr))); } ASSERT(length >= len); memcpy(buffer, data.data(), len); - (*ctr) += 1; + *ctr += 1; return Api::SysCallSizeResult{ssize_t(len), 0}; })); } From ca38787a99eefed7060d82763b992a92d0dcc558 Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Fri, 14 Aug 2020 11:18:49 -0700 Subject: [PATCH 6/9] fix ws Signed-off-by: Sotiris Nanopoulos --- test/extensions/filters/listener/http_inspector/BUILD | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/listener/http_inspector/BUILD b/test/extensions/filters/listener/http_inspector/BUILD index d4c49021da09..15f753a89b8e 100644 --- a/test/extensions/filters/listener/http_inspector/BUILD +++ b/test/extensions/filters/listener/http_inspector/BUILD @@ -17,7 +17,7 @@ envoy_extension_cc_test( extension_name = "envoy.filters.listener.http_inspector", #TODO(davinci26): The test passes on Windows *but* http inspector # *used* to rely on Event::FileTriggerType::Edge and we got away with it - # because we mock the dispatcher. Need to verify that the scenario is + # because we mock the dispatcher. Need to verify that the scenario is # actually working. tags = ["fails_on_windows"], deps = [ From 271ca3931afc9039a29f35c8a02486c1807fbac5 Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Fri, 14 Aug 2020 12:05:04 -0700 Subject: [PATCH 7/9] fix spelling Signed-off-by: Sotiris Nanopoulos --- .../filters/listener/http_inspector/http_inspector_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/listener/http_inspector/http_inspector_test.cc b/test/extensions/filters/listener/http_inspector/http_inspector_test.cc index 19e909ec6a77..cc1ff48aad57 100644 --- a/test/extensions/filters/listener/http_inspector/http_inspector_test.cc +++ b/test/extensions/filters/listener/http_inspector/http_inspector_test.cc @@ -568,7 +568,7 @@ TEST_F(HttpInspectorTest, MultipleReadsHttp1BadProtocol) { TEST_F(HttpInspectorTest, Http1WithLargeRequestLine) { // Verify that the http inspector can detect http requests - // with large request line even when they are splitted over + // with large request line even when they are split over // multiple recv calls. init(); absl::string_view method = "GET", http = "/index HTTP/1.0\r"; From 454a418be75312d5e1497f44710a9c9ab8c53af9 Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Fri, 14 Aug 2020 13:00:05 -0700 Subject: [PATCH 8/9] fix compilation Signed-off-by: Sotiris Nanopoulos --- .../filters/listener/http_inspector/http_inspector_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/listener/http_inspector/http_inspector_test.cc b/test/extensions/filters/listener/http_inspector/http_inspector_test.cc index cc1ff48aad57..6dd564dbe79b 100644 --- a/test/extensions/filters/listener/http_inspector/http_inspector_test.cc +++ b/test/extensions/filters/listener/http_inspector/http_inspector_test.cc @@ -594,7 +594,7 @@ TEST_F(HttpInspectorTest, Http1WithLargeRequestLine) { int) -> Api::SysCallSizeResult { size_t len = (*ctr); if (num_loops == 2) { - ASSERT(*ctx != 3) + ASSERT(*ctx != 3); len = size_t(Config::MAX_INSPECT_SIZE / (3 - (*ctr))); } ASSERT(length >= len); From 86ff5855957c087d786f75bf7be84d6591395d6e Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Fri, 14 Aug 2020 13:51:34 -0700 Subject: [PATCH 9/9] Never skip local build again Signed-off-by: Sotiris Nanopoulos --- .../filters/listener/http_inspector/http_inspector_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/listener/http_inspector/http_inspector_test.cc b/test/extensions/filters/listener/http_inspector/http_inspector_test.cc index 6dd564dbe79b..2e1bc0d5ad02 100644 --- a/test/extensions/filters/listener/http_inspector/http_inspector_test.cc +++ b/test/extensions/filters/listener/http_inspector/http_inspector_test.cc @@ -594,7 +594,7 @@ TEST_F(HttpInspectorTest, Http1WithLargeRequestLine) { int) -> Api::SysCallSizeResult { size_t len = (*ctr); if (num_loops == 2) { - ASSERT(*ctx != 3); + ASSERT(*ctr != 3); len = size_t(Config::MAX_INSPECT_SIZE / (3 - (*ctr))); } ASSERT(length >= len);