abseil: force using non-std types

[mattklein123]

Force using absl optional/string_view in most places. Allow use of std::string_view where
required for extensions, in particular WASM.

Risk Level: Low
Testing: Existing tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

[repokitteh-read-only]

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: #16029 was opened by mattklein123.

see: more, trace.

[mattklein123]

@PiotrSikora this fails with:

external/proxy_wasm_cpp_host/include/proxy-wasm/context.h:219:8: note: hidden overloaded virtual function 'proxy_wasm::ContextBase::error' declared here: type mismatch at 1st parameter ('std::string_view' (aka 'basic_string_view<char>') vs 'absl::string_view')
  void error(std::string_view message) override {

Is there any chance the WASM cpp code could use absl::string_view so we can land this change?

[PiotrSikora]

@PiotrSikora this fails with:

external/proxy_wasm_cpp_host/include/proxy-wasm/context.h:219:8: note: hidden overloaded virtual function 'proxy_wasm::ContextBase::error' declared here: type mismatch at 1st parameter ('std::string_view' (aka 'basic_string_view<char>') vs 'absl::string_view')
  void error(std::string_view message) override {

Is there any chance the WASM cpp code could use absl::string_view so we can land this change?

We used to have support for abseil as an optional dependency, but it was removed after Envoy moved to C++17, since there was no need for it anymore (see: proxy-wasm/proxy-wasm-cpp-host#41 and proxy-wasm/proxy-wasm-cpp-sdk#38).

I don't mind re-adding abseil to Proxy-Wasm C++ Host if Envoy needs it, but I'm afraid that it's going to turn into a need for abseil in Proxy-Wasm C++ SDK, which definitely shouldn't have a hard dependency on it, so we need to add it as an optional dependency, and default to std types.

cc @mathetake

[mattklein123]

I don't mind re-adding abseil to Proxy-Wasm C++ Host if Envoy needs it, but I'm afraid that it's going to turn into a need for abseil in Proxy-Wasm C++ SDK, which definitely shouldn't have a hard dependency on it, so we need to add it as an optional dependency, and default to std types.

Sure this sounds reasonable to me. @envoyproxy/maintainers what do we think about this change overall? IMO it's worth it to pin to the internal types and not using the std types, especially given the recent security issue with string_view.

[ggreenway]

I'm in favor of this change. Having different implementations on different platforms, with different behavior (eg nullptr allowed vs not allowed), is not good.

[jmarantz]

@PiotrSikora with C++-17 I would think we'd still want absl as a dependency for some of the functionality it adds beyond what's being pulled into std::.

Examples off the top of my head

• better mutexes/condvars/await including contention detection
• better time abstraction (though admittedly Envoy doesn't use that much)
• string joining, splitting, replace, concat, etc
• spans
• variants
• optionals

I think not all of that is destined to ever go into std::.

[PiotrSikora]

I'm in favor of this change. Having different implementations on different platforms, with different behavior (eg nullptr allowed vs not allowed), is not good.

Note that abseil is the different one here, the rest of the C++ world doesn't accept nullptr as std::string_view.

AFAICT, MSVC doesn't allow it either (see: string_view constructor length check nullptr), so I'm a bit puzzled by your comment regarding different behavior on different platforms.

FWIW, I agree that allowing nullptr is a safer behavior, but that's not what C++17 defines.

@PiotrSikora with C++-17 I would think we'd still want absl as a dependency for some of the functionality it adds beyond what's being pulled into std::.

Examples off the top of my head

• better mutexes/condvars/await including contention detection
• better time abstraction (though admittedly Envoy doesn't use that much)
• string joining, splitting, replace, concat, etc
• spans
• variants
• optionals

I think not all of that is destined to ever go into std::.

In Proxy-Wasm C++ Host code (which my comment was referring to), we were only using absl::string_view and absl::optional, both of which are included in C++17, so the extra dependency didn't provide any value.

I'd love to use absl::span<uint8_t> instead of absl::string_view for non-string data, but Envoy doesn't use that, so it would be a bunch of unnecessary conversions in the Host code.

But like I said, I'm fine with accepting abseil as an optional dependency if Envoy switches back to non-std types, but that's the only justification for having it there at this point.

[ggreenway]

your comment regarding different behavior on different platforms

Because on some platforms the abseil implementation was being used, and on some the c++17 std library was being used. IIRC, envoy-mobile needed the abseil implementation somewhere.

I am not aware of any c++ std library implementation that accepts a nullptr in this case without crashing.

[lizan]

I am not aware of any c++ std library implementation that accepts a nullptr in this case without crashing.

libstdc++ does allow nullptr without crashing.

[antoniovicente]

I am not aware of any c++ std library implementation that accepts a nullptr in this case without crashing.

libstdc++ does allow nullptr without crashing.

Can you expand on this? I was lead to believe strlen(nullptr) results in a crash. Does the libstdc++ string_view avoid the call to strlen or have a strlen that checks for nullptr?

[mattklein123]

It sounds like we don't have consensus here on whether this change is wanted or not. Do we want to adhere more to the C++ standard? Or use the built-in safety/consistency of the abseil version? If we stick with the C++ standard is there some way clang-tidy can catch a C string being passed into a string_view without wrapping?

[ggreenway]

It sounds like we don't have consensus here on whether this change is wanted or not. Do we want to adhere more to the C++ standard? Or use the built-in safety/consistency of the abseil version? If we stick with the C++ standard is there some way clang-tidy can catch a C string being passed into a string_view without wrapping?

I prefer the safer version. I'm not concerned about the performance cost of an extra nullptr check.

I don't think static analysis will in practice help much, because it doesn't know the difference between a possibly null char* and a guaranteed-not-null char*.

[antoniovicente]

I think that using the safer version for now is a good idea. Bonus points for reaching out to abseil about the possibility of using the nullptr safe version in the StrCat implementation.

[mattklein123]

OK sounds like we are roughly agreed that we should proceed with this change. @PiotrSikora are you willing to bring back abseil support in the WASM code (as a compile time option)?

[mattklein123]

OK it sounds like we have some consensus here on moving forward. @PiotrSikora are you willing to bring back the option in proxy-wasm to use abseil and then I can finish this change? Or would you prefer I do that change?

[PiotrSikora]

@mattklein123 if you could do it, that would be great. Thanks!

[mattklein123]

I'm coming back to this and now wasm_cpp is not the only problem, the skywalking SDK also has the same problem. I'm concerned that we may be playing whackamole here given that we support C++17. We would essentially have to force all deps to use abseil. Now that I think about it I'm not sure this is reasonable. @lizan @antoniovicente @ggreenway thoughts?

[PiotrSikora]

I'm coming back to this and now wasm_cpp is not the only problem, the skywalking SDK also has the same problem. I'm concerned that we may be playing whackamole here given that we support C++17. We would essentially have to force all deps to use abseil. Now that I think about it I'm not sure this is reasonable. @lizan @antoniovicente @ggreenway thoughts?

I think that simply allowing use of std::string_view, so that Envoy core uses absl::string_view, and extensions that inherit from classes in external C++ libraries (like Wasm and Skywalking) use std::string_view, should also work?

[mattklein123]

I think that simply allowing use of std::string_view, so that Envoy core uses absl::string_view, and extensions that inherit from classes in external C++ libraries (like Wasm and Skywalking) use std::string_view, should also work?

Yes I agree, that would be a fallback position and if we want to go that route that would be fine. I think we would have to relax or linting or NOLINT all sites that use it in the core code, but I can look into that.

[mattklein123]

@PiotrSikora @ggreenway @lizan @antoniovicente PTAL and let me know what you think. I'm happy to drop this if we think it's not worthwhile, but I think this is relatively clean and gets the job done for the most part.

[PiotrSikora]

Thanks, I think this makes the most sense!

[ggreenway]

LGTM, modulo @PiotrSikora 's request for a comment in the linter

[ggreenway]

Merge conflict.

/wait

[mattklein123]

@ggreenway @PiotrSikora updated PTAL