Add (http2|spdy)_string_utils_impl to QUICHE platform implementation. by wu-bin · Pull Request #6198 · envoyproxy/envoy

wu-bin · 2019-03-06T21:50:24Z

Description:

Add (http2|spdy)_string_utils_impl to QUICHE platform implementation.

Enabled their existing test in QUICHE.

Risk Level: minimal: code not used yet
Testing:

bazel test test/extensions/quic_listeners/quiche/platform:quic_platform_test --test_output=all --define quiche=enabled --experimental_remap_main_repo

bazel test @com_googlesource_quiche//:spdy_platform_test --test_output=all --define quiche=enabled

bazel test @com_googlesource_quiche//:http2_platform_test --test_output=all --define quiche=enabled

Docs Changes: none
Release Notes: none
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Bin Wu <wub@google.com>

…g_utils_impl Signed-off-by: Bin Wu <wub@google.com>

assert_lib. Signed-off-by: Bin Wu <wub@google.com>

wu-bin · 2019-03-06T22:34:21Z

/assign @mpwarres

mpwarres · 2019-03-07T03:34:58Z

-        # "quiche/spdy/platform/api/spdy_mem_slice.h",
-        # "quiche/spdy/platform/api/spdy_string_utils.h",
-    ],
+    ] + envoy_select_quiche(


Why is envoy_select_quiche needed here? My understanding from when it was first introduced was that it was only needed to avoid compiling targets depending on QUICHE under @envoy//test/..., and that for non-test targets, the omission of QUICHE from source/extensions/extensions_build_config.bzl would keep them from being compiled (e.g. see this comment). Did this turn out to not be the case?

I have used to put everything(hdrs, srcs, deps) that depends on envoy into envoy_select_quiche. Last time I tried, I think ci still fails without it.

Ok, got it, SG. Separately, I'll look into if there's anything that can be done to reduce that need, but that's orthogonal to this CL. (And I'm finding it affects the flags_impl CL as well.)

For my education, why is this header file special vs. others in quich/spdy/platform/api etc.?

This file depends on "string_utils_lib" which depends on envoy's "//source/common/common:assert_lib".
Other files in this library does not depend on envoy.

I'm not sure how that dependency is reflected in this code though; where is the line that expresses this dependency?

Also, can you fix the bracket style as per previous comments.

quiche/spdy/platform/api/spdy_string_utils.h includes source/extensions/quic_listeners/quiche/platform/spdy_string_utils_impl.h, which is provided by build rule "spdy_platform_impl_lib", which depends on "string_utils_lib".

Fixed all single element lists in bazel/external/quiche.BUILD and source/extensions/quic_listeners/quiche/platform/BUILD.

mpwarres · 2019-03-07T03:47:29Z

+
+namespace http2 {
+
+std::string Http2HexDumpImpl(absl::string_view data) {


There's code that's identical to this in QuicTextUtilsImpl::HexDump(), and also later on in SpdyHexDumpImpl. How about having a single copy, in (say) some platform/string_utils.cc file, that all platform impls then delegate to?

Good idea! Done.

mpwarres · 2019-03-07T04:00:19Z

+  return 0;
+}
+
+bool SpdyHexDecodeToUInt32Impl(absl::string_view data, uint32_t* out) {


Can absl::HexStringToBytes() be used here?

…g_utils_impl Signed-off-by: Bin Wu <wub@google.com>

from quic/spdy/http2 platform impls. Signed-off-by: Bin Wu <wub@google.com>

wu-bin

Thanks Mike for the review. I have added a single string_utils_lib and used it from quic/spdy/http2 string apis.

wu-bin · 2019-03-13T13:46:49Z

-        # "quiche/spdy/platform/api/spdy_mem_slice.h",
-        # "quiche/spdy/platform/api/spdy_string_utils.h",
-    ],
+    ] + envoy_select_quiche(


I have used to put everything(hdrs, srcs, deps) that depends on envoy into envoy_select_quiche. Last time I tried, I think ci still fails without it.

wu-bin · 2019-03-13T15:28:37Z

+
+namespace http2 {
+
+std::string Http2HexDumpImpl(absl::string_view data) {


Good idea! Done.

Signed-off-by: Bin Wu <wub@google.com>

mpwarres

LGTM, thanks!

/assign @alyssawilk

mpwarres · 2019-03-13T21:18:44Z

-        # "quiche/spdy/platform/api/spdy_mem_slice.h",
-        # "quiche/spdy/platform/api/spdy_string_utils.h",
-    ],
+    ] + envoy_select_quiche(


Ok, got it, SG. Separately, I'll look into if there's anything that can be done to reduce that need, but that's orthogonal to this CL. (And I'm finding it affects the flags_impl CL as well.)

htuch

Looks good, a couple of comments. Thanks

htuch · 2019-03-19T04:04:10Z

-        # "quiche/spdy/platform/api/spdy_mem_slice.h",
-        # "quiche/spdy/platform/api/spdy_string_utils.h",
-    ],
+    ] + envoy_select_quiche(


For my education, why is this header file special vs. others in quich/spdy/platform/api etc.?

htuch · 2019-03-19T04:05:21Z

-        "//source/common/http:utility_lib",
-    ]),
+    deps =
+        envoy_select_quiche([


Nit: please stick to normal Bazel style and have deps = envoy_select_quiche(... with no line break.

htuch · 2019-03-19T04:05:56Z


+envoy_cc_library(
+    name = "string_utils_lib",
+    srcs = [


Nit: single line lists should just be srcs = ["string_utils.c"], for conciseness.

…g_utils_impl Signed-off-by: Bin Wu <wub@google.com>

Signed-off-by: Bin Wu <wub@google.com>

wu-bin

Thanks for the review!

wu-bin · 2019-03-19T13:14:50Z

-        # "quiche/spdy/platform/api/spdy_mem_slice.h",
-        # "quiche/spdy/platform/api/spdy_string_utils.h",
-    ],
+    ] + envoy_select_quiche(


This file depends on "string_utils_lib" which depends on envoy's "//source/common/common:assert_lib".
Other files in this library does not depend on envoy.

wu-bin · 2019-03-19T13:17:16Z


+envoy_cc_library(
+    name = "string_utils_lib",
+    srcs = [


wu-bin · 2019-03-19T13:19:47Z

-        "//source/common/http:utility_lib",
-    ]),
+    deps =
+        envoy_select_quiche([


output. Use it in quiche::Base64Encode. Signed-off-by: Bin Wu <wub@google.com>

htuch · 2019-03-19T14:46:15Z

-        # "quiche/spdy/platform/api/spdy_mem_slice.h",
-        # "quiche/spdy/platform/api/spdy_string_utils.h",
-    ],
+    ] + envoy_select_quiche(


I'm not sure how that dependency is reflected in this code though; where is the line that expresses this dependency?

Also, can you fix the bracket style as per previous comments.

htuch · 2019-03-19T14:46:49Z

+envoy_cc_test(
+    name = "http2_platform_test",
+    srcs = envoy_select_quiche(
+        [


Please merge make the use of brackets match other places in Envoy. Prefer compact style as with google3 C++ and BUILD files.

htuch · 2019-03-19T14:49:00Z

+      Envoy::Base64::encode(reinterpret_cast<const char*>(data), data_len, /*add_padding=*/false);
+}
+
+std::string HexDump(absl::string_view data) {


Can you reuse https://github.com/envoyproxy/envoy/blob/master/source/common/common/hex.cc and https://github.com/envoyproxy/envoy/blob/master/source/common/common/hex.h here?

They can not be used since they are not hexdump, rather they are equivalent to absl's HexStringToBytes and BytesToHexString functions.

I could move the body of HexDump to common/hex.cc and use it from here, the caveat is that the format of its output must not change, because quiche unit test expects the same output format(example) from all platforms' implementations. Do you prefer to do it that way?

Are these only test functions? I don't mind adding them if they are only used for this, but should then have some comments or change their name. I think a few things:

It's helpful to have these kinds of utils, so it would be nice to have them around in Envoy.

It would ideally have a bunch of tests to ensure they behave as intended
*I'd prefer we did this as much as possible as pure C++, to avoid any of the evil of C string and buffer management.

Thanks for the offline chat, Harvey. I have

Changed the visibility of "string_utils_lib" to private.

Opened Cleanup BUILD visibility for quiche build rules. #6319 to cleanup the visibility of other quiche build rules.

htuch · 2019-03-19T14:49:46Z

+
+  // Pad with leading zeros.
+  std::string data_padded(8u, '0');
+  memcpy(&data_padded[8u - data.size()], data.data(), data.size());


I have a major aversion to any use of memcpy or C string utilities unless absolutely needed. This is scary from a security perspective.

Changed to string::insert instead.

htuch · 2019-03-19T14:50:32Z

+
+// TODO(danzh): Fill out SpdyMemSliceImpl.
+//
+// SpdyMemSliceImpl wraps a reference counted MemSlice and only provides partial


Does absl or C++14 give us this? Where is the implementation?

This is just a header file included by spdy_string_utils.h. I add it to make this PR compile. @danzh2010 is working on the implementation in a separate PR.

Signed-off-by: Bin Wu <wub@google.com>

wu-bin

Thanks Harvey, PTAL.

wu-bin · 2019-03-19T14:56:51Z

+
+// TODO(danzh): Fill out SpdyMemSliceImpl.
+//
+// SpdyMemSliceImpl wraps a reference counted MemSlice and only provides partial


This is just a header file included by spdy_string_utils.h. I add it to make this PR compile. @danzh2010 is working on the implementation in a separate PR.

wu-bin · 2019-03-19T15:15:41Z

+      Envoy::Base64::encode(reinterpret_cast<const char*>(data), data_len, /*add_padding=*/false);
+}
+
+std::string HexDump(absl::string_view data) {


They can not be used since they are not hexdump, rather they are equivalent to absl's HexStringToBytes and BytesToHexString functions.

I could move the body of HexDump to common/hex.cc and use it from here, the caveat is that the format of its output must not change, because quiche unit test expects the same output format(example) from all platforms' implementations. Do you prefer to do it that way?

wu-bin · 2019-03-19T15:33:30Z

-        # "quiche/spdy/platform/api/spdy_mem_slice.h",
-        # "quiche/spdy/platform/api/spdy_string_utils.h",
-    ],
+    ] + envoy_select_quiche(


quiche/spdy/platform/api/spdy_string_utils.h includes source/extensions/quic_listeners/quiche/platform/spdy_string_utils_impl.h, which is provided by build rule "spdy_platform_impl_lib", which depends on "string_utils_lib".

Fixed all single element lists in bazel/external/quiche.BUILD and source/extensions/quic_listeners/quiche/platform/BUILD.

wu-bin · 2019-03-19T15:33:45Z

+envoy_cc_test(
+    name = "http2_platform_test",
+    srcs = envoy_select_quiche(
+        [


wu-bin · 2019-03-19T15:46:40Z

+
+  // Pad with leading zeros.
+  std::string data_padded(8u, '0');
+  memcpy(&data_padded[8u - data.size()], data.data(), data.size());


Changed to string::insert instead.

Signed-off-by: Bin Wu <wub@google.com>

wu-bin · 2019-03-20T13:08:19Z

/retest

repokitteh-read-only · 2019-03-20T13:08:23Z

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #6198 (comment) was created by @wu-bin.

see: more, trace.

htuch

Thanks!

wu-bin added 3 commits March 6, 2019 09:33

Initial checkin of (http2|spdy)_string_utils_impl.

5cfe801

Signed-off-by: Bin Wu <wub@google.com>

Merge remote-tracking branch 'upstream/master' into h2_platform_strin…

ba12210

…g_utils_impl Signed-off-by: Bin Wu <wub@google.com>

Add envoy_select_quiche to spdy_string_utils since it depends on Envoy's

525c6b5

assert_lib. Signed-off-by: Bin Wu <wub@google.com>

wu-bin requested review from alyssawilk and mattklein123 as code owners March 6, 2019 21:50

repokitteh-read-only Bot assigned mpwarres Mar 6, 2019

mpwarres reviewed Mar 7, 2019

View reviewed changes

wu-bin added 2 commits March 13, 2019 09:48

Merge remote-tracking branch 'upstream/master' into h2_platform_strin…

bafc303

…g_utils_impl Signed-off-by: Bin Wu <wub@google.com>

Update per review comments: add a single string_utils_lib and use it

433db7e

from quic/spdy/http2 platform impls. Signed-off-by: Bin Wu <wub@google.com>

wu-bin commented Mar 13, 2019

View reviewed changes

wu-bin added 2 commits March 13, 2019 13:08

Try to fix clang_tidy error.

b259f56

Signed-off-by: Bin Wu <wub@google.com>

Second attempt to make clang_tidy happy.

e5bed7c

Signed-off-by: Bin Wu <wub@google.com>

mpwarres previously approved these changes Mar 13, 2019

View reviewed changes

htuch self-assigned this Mar 18, 2019

htuch suggested changes Mar 19, 2019

View reviewed changes

wu-bin added 2 commits March 19, 2019 09:15

Merge remote-tracking branch 'upstream/master' into h2_platform_strin…

a2bf207

…g_utils_impl Signed-off-by: Bin Wu <wub@google.com>

Update per review comments.

f451913

Signed-off-by: Bin Wu <wub@google.com>

wu-bin dismissed mpwarres’s stale review via f451913 March 19, 2019 13:43

wu-bin commented Mar 19, 2019

View reviewed changes

Add a overload of Base64::encode function to not to add padding to the

3c6c543

output. Use it in quiche::Base64Encode. Signed-off-by: Bin Wu <wub@google.com>

htuch suggested changes Mar 19, 2019

View reviewed changes

htuch added the waiting label Mar 19, 2019

Update per review comments.

521d742

Signed-off-by: Bin Wu <wub@google.com>

repokitteh-read-only Bot removed the waiting label Mar 19, 2019

wu-bin commented Mar 19, 2019

View reviewed changes

Change the visibility of "string_utils_lib" to private.

1c04128

Signed-off-by: Bin Wu <wub@google.com>

htuch approved these changes Mar 20, 2019

View reviewed changes

htuch merged commit 36f39c7 into envoyproxy:master Mar 20, 2019


		namespace http2 {

		std::string Http2HexDumpImpl(absl::string_view data) {

Conversation

wu-bin commented Mar 6, 2019

Uh oh!

wu-bin commented Mar 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-bin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpwarres left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wu-bin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment