-
Notifications
You must be signed in to change notification settings - Fork 5.5k
build: Introduce GURL with shimmed ICU #13808
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.
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
Changes from 14 commits
c3d45f6
da2dd95
34ee5c4
f9add04
413a53e
0e622c4
40a65c8
4c639f1
3409bf8
542c300
40742b0
74ec92c
b320b8e
a69238f
ce9abef
127968a
e36e797
86898d2
429afb3
cbc6c36
6263208
c005164
9adc264
5550e24
f285916
0945e28
feb9b80
99e5f2b
d4fc6f2
5068f77
b2a02c4
71e6003
241c6f4
52055e8
3fe1379
86c2693
5aafdde
96ec874
b58e957
64645dc
91758d9
526b5ed
24e3c72
c5ef876
77a8099
f4342d1
1c3a923
87ba219
60e5c13
cbc1c09
e8d43dd
5e06616
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| # TODO(dio): Consider to remove this patch when we solely compile the project using clang-cl. | ||
| # Tracked in https://github.com/envoyproxy/envoy/issues/11974. | ||
|
|
||
| diff --git a/base/compiler_specific.h b/base/compiler_specific.h | ||
| index 0cd36dc..8c4cbd4 100644 | ||
| --- a/base/compiler_specific.h | ||
| +++ b/base/compiler_specific.h | ||
| @@ -7,10 +7,6 @@ | ||
|
|
||
| #include "build/build_config.h" | ||
|
|
||
| -#if defined(COMPILER_MSVC) && !defined(__clang__) | ||
| -#error "Only clang-cl is supported on Windows, see https://crbug.com/988071" | ||
| -#endif | ||
| - | ||
| // Annotate a variable indicating it's ok if the variable is not used. | ||
| // (Typically used to silence a compiler warning when the assignment | ||
| // is important for some other reason.) | ||
| @@ -55,8 +51,12 @@ | ||
| // prevent code folding, see gurl_base::debug::Alias. | ||
| // Use like: | ||
| // void NOT_TAIL_CALLED FooBar(); | ||
| -#if defined(__clang__) && __has_attribute(not_tail_called) | ||
| +#if defined(__clang__) | ||
| +#if defined(__has_attribute) | ||
| +#if __has_attribute(not_tail_called) | ||
| #define NOT_TAIL_CALLED __attribute__((not_tail_called)) | ||
| +#endif | ||
| +#endif | ||
| #else | ||
| #define NOT_TAIL_CALLED | ||
| #endif | ||
| @@ -226,7 +226,9 @@ | ||
| #endif | ||
| #endif | ||
|
|
||
| -#if defined(__clang__) && __has_attribute(uninitialized) | ||
| +#if defined(__clang__) | ||
| +#if defined(__has_attribute) | ||
| +#if __has_attribute(uninitialized) | ||
| // Attribute "uninitialized" disables -ftrivial-auto-var-init=pattern for | ||
| // the specified variable. | ||
| // Library-wide alternative is | ||
| @@ -257,6 +259,8 @@ | ||
| // E.g. platform, bot, benchmark or test name in patch description or next to | ||
| // the attribute. | ||
| #define STACK_UNINITIALIZED __attribute__((uninitialized)) | ||
| +#endif | ||
| +#endif | ||
| #else | ||
| #define STACK_UNINITIALIZED | ||
| #endif |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| load( | ||
|
htuch marked this conversation as resolved.
|
||
| "//bazel:envoy_build_system.bzl", | ||
| "envoy_cc_test", | ||
| "envoy_package", | ||
| ) | ||
|
|
||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| envoy_package() | ||
|
|
||
| envoy_cc_test( | ||
| name = "test", | ||
| srcs = ["test.cc"], | ||
| external_deps = [ | ||
| "googleurl", | ||
| ], | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| #include "url/gurl.h" | ||
|
|
||
| #include "gtest/gtest.h" | ||
|
|
||
| namespace { | ||
|
|
||
| // Basic smoke test to ensure that GURL (with shimmed ICU) works properly. | ||
| TEST(GoogleUrl, SmokeTest) { | ||
| GURL url("https://example.org/test?foo=bar#section"); | ||
| EXPECT_TRUE(url.is_valid()); | ||
| EXPECT_EQ(url.scheme(), "https"); | ||
| EXPECT_EQ(url.host(), "example.org"); | ||
| EXPECT_EQ(url.EffectiveIntPort(), 443); | ||
| EXPECT_EQ(url.path(), "/test"); | ||
| EXPECT_EQ(url.query(), "foo=bar"); | ||
| EXPECT_EQ(url.ref(), "section"); | ||
|
|
||
| GURL punycode("https://xn--c1yn36f.example"); | ||
| EXPECT_TRUE(punycode.is_valid()); | ||
|
|
||
| GURL percent_encoded_valid("https://%20.example"); | ||
| EXPECT_TRUE(percent_encoded_valid.is_valid()); | ||
|
|
||
| // Ensure ICU shim is functioning correctly, i.e. not crashing and resulting invalid parsed URL. | ||
| GURL idn_url("https://\xe5\x85\x89.example/"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also add tests that have %-encoded sequences in the host name as well as tests with hostnames that are not DNS RFC compliant, please?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added some. but I think my creativity is lacking. If you have some examples, please let me know. Thank you!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Btw, this seems like a great (and not so difficult) candidate for a fuzzer that would catch GURL crashes
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @asraa thanks! I added the setup. Please take a look when you have time. |
||
| EXPECT_FALSE(idn_url.is_valid()); | ||
|
|
||
| GURL idn_with_percent("https://%Da%aa.example"); | ||
| EXPECT_FALSE(idn_with_percent.is_valid()); | ||
|
|
||
| GURL with_bracket("http://[wwww].example"); | ||
| EXPECT_FALSE(with_bracket.is_valid()); | ||
| } | ||
|
|
||
| } // namespace | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| load("@rules_cc//cc:defs.bzl", "cc_library", "cc_test") | ||
|
|
||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| cc_library( | ||
| name = "common", | ||
| hdrs = [ | ||
| "common/unicode/uidna.h", | ||
| "common/unicode/utypes.h", | ||
| ], | ||
| includes = ["common"], | ||
| visibility = ["//visibility:public"], | ||
| ) | ||
|
|
||
| cc_test( | ||
| name = "common_test", | ||
| srcs = ["common_test.cc"], | ||
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| ":common", | ||
| ], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| workspace(name = "org_unicode_icuuc") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| #pragma once | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @danzh2010 @alyssawilk do you know how reasonable it will be to add proper Chromium URL support for eliding unicode translation? I think this shim is fine for now, but ideally we can move away from needing to workarounds like this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure, but cc @DavidSchinazi @ianswett just to have on our list.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's definitely doable to pull in the full Chromium IDN logic, but that has significant impact on binary size - not sure what the full set of design goals and requirements is here though
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DavidSchinazi I think probably what we need is a way to build GURL without Unicode support (probably via defining build flag etc). Is that something viable?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dio I think that should be doable by using IDN from the OS: |
||
|
|
||
| #include <iostream> | ||
| #include "unicode/utypes.h" | ||
|
|
||
| // Initialize UIDNAInfo object with UIDNA_ERROR_DISALLOWED as its errors (any non-zero value should | ||
| // work). | ||
| #define UIDNA_INFO_INITIALIZER \ | ||
| { 0x80 } | ||
|
|
||
| // This enum is declared to satisfy uidna_openUTS46 when initializing UIDNAWrapper | ||
| // https://quiche.googlesource.com/googleurl/+/ef0d23689e240e6c8de4c3a5296b209128c87373/url/url_idna_icu.cc#50. | ||
| enum { UIDNA_CHECK_BIDI = 4 }; | ||
|
|
||
| // The fake UIDNA. | ||
| struct UIDNA {}; | ||
|
|
||
| // The fake UIDNAInfo. | ||
| struct UIDNAInfo { | ||
| uint32_t errors; | ||
| }; | ||
|
|
||
| // This is called by IDNToASCII | ||
| // (https://quiche.googlesource.com/googleurl/+/ef0d23689e240e6c8de4c3a5296b209128c87373/url/url_idna_icu.cc#85) | ||
| // which is responsible for converting the Unicode input representing a hostname to ASCII using IDN | ||
| // rules. In this shimmed implementation, we always set the error code to U_ILLEGAL_ARGUMENT_ERROR | ||
| // and info errors to UIDNA_ERROR_DISALLOWED, hence the coversion always fails. | ||
| int32_t uidna_nameToASCII(const UIDNA*, const UChar*, int32_t, UChar*, int32_t, UIDNAInfo* info, | ||
| UErrorCode* error_code) { | ||
| *error_code = U_ILLEGAL_ARGUMENT_ERROR; | ||
| return 0; | ||
| } | ||
|
|
||
| // This is called inside the UIDNAWrapper | ||
| // (https://quiche.googlesource.com/googleurl/+/ef0d23689e240e6c8de4c3a5296b209128c87373/url/url_idna_icu.cc#50) | ||
| // and should be accessed only through GetUIDNA | ||
| // (https://quiche.googlesource.com/googleurl/+/ef0d23689e240e6c8de4c3a5296b209128c87373/url/url_idna_icu.cc#66). | ||
| // This pattern is used inside GURL to initialize a static value at first use. It returns an | ||
| // instance of UIDNA to satisfy the check (GURL_DCHECK(uidna != nullptr);) here: | ||
| // https://quiche.googlesource.com/googleurl/+/ef0d23689e240e6c8de4c3a5296b209128c87373/url/url_idna_icu.cc#89. | ||
| UIDNA* uidna_openUTS46(uint32_t options, UErrorCode* error_code) { return new UIDNA; } | ||
|
|
||
| // While this is never be called, it needs to be defined since: | ||
| // https://quiche.googlesource.com/googleurl/+/ef0d23689e240e6c8de4c3a5296b209128c87373/url/url_idna_icu.cc#53. | ||
| const char* u_errorName(UErrorCode) { return "U_ILLEGAL_ARGUMENT_ERROR"; } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| #pragma once | ||
|
|
||
| #include <cstdint> | ||
| #include <iostream> | ||
|
|
||
| using UBool = bool; | ||
| using UChar = uint16_t; | ||
|
|
||
| enum UErrorCode { | ||
| // This value is used for initializing error code in this line: | ||
| // https://quiche.googlesource.com/googleurl/+/ef0d23689e240e6c8de4c3a5296b209128c87373/url/url_idna_icu.cc#91. | ||
| U_ZERO_ERROR = 0, | ||
|
|
||
| // This makes info.errors in this line: | ||
| // https://quiche.googlesource.com/googleurl/+/ef0d23689e240e6c8de4c3a5296b209128c87373/url/url_idna_icu.cc#102 | ||
| // to be non-zero. | ||
| U_ILLEGAL_ARGUMENT_ERROR = 1, | ||
|
|
||
| // Required in this line: | ||
| // https://quiche.googlesource.com/googleurl/+/ef0d23689e240e6c8de4c3a5296b209128c87373/url/url_idna_icu.cc#102. | ||
| U_BUFFER_OVERFLOW_ERROR = 15 | ||
| }; | ||
|
|
||
| // This is called inside IDNToASCII | ||
| // (https://quiche.googlesource.com/googleurl/+/ef0d23689e240e6c8de4c3a5296b209128c87373/url/url_idna_icu.cc#95), | ||
| // this should always return false. | ||
| static inline UBool U_SUCCESS(UErrorCode) { return false; } | ||
|
|
||
| // This is called by UIDNAWrapper constructor | ||
| // (https://quiche.googlesource.com/googleurl/+/ef0d23689e240e6c8de4c3a5296b209128c87373/url/url_idna_icu.cc#51). | ||
| // This should always return false, since the initialization of UIDNAWrapper should always be | ||
| // successful. | ||
| static inline UBool U_FAILURE(UErrorCode) { return false; } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| // Basic smoke test to ensure that ICU shim works properly. | ||
|
|
||
| #include <iostream> | ||
| #include "unicode/uidna.h" | ||
|
|
||
| #define ASSERT_EQ(v1, v2) \ | ||
| if ((v1) != (v2)) { \ | ||
| std::cerr << "Expected equality of" << std::endl \ | ||
| << " " << #v1 << " (equal to " << (v1) << ")" << std::endl \ | ||
| << "and" << std::endl \ | ||
| << " " << #v2 << " (equal to " << (v2) << ")" << std::endl; \ | ||
| return 1; \ | ||
| } | ||
|
|
||
| int main(int argc, char** argv) { | ||
| { | ||
| UErrorCode err = U_ZERO_ERROR; | ||
| auto uidna = uidna_openUTS46(0, &err); | ||
|
|
||
| ASSERT_EQ(uidna != nullptr, true); | ||
| ASSERT_EQ(err, U_ZERO_ERROR); | ||
|
|
||
| delete uidna; | ||
| } | ||
|
|
||
| { | ||
| UErrorCode err = U_ZERO_ERROR; | ||
| UIDNAInfo info = UIDNA_INFO_INITIALIZER; | ||
| uidna_nameToASCII(nullptr, nullptr, 0, nullptr, 0, &info, &err); | ||
|
|
||
| ASSERT_EQ(info.errors, 0x80); | ||
| ASSERT_EQ(err, U_ILLEGAL_ARGUMENT_ERROR); | ||
| } | ||
|
|
||
| { | ||
| UErrorCode err = U_ZERO_ERROR; | ||
| UIDNAInfo info = UIDNA_INFO_INITIALIZER; | ||
| UChar data[] = {'1', '2', '3'}; | ||
| UChar* src = &data[0]; | ||
| uidna_nameToASCII(nullptr, src, 0, nullptr, *src, &info, &err); | ||
|
|
||
| ASSERT_EQ(info.errors, 0x80); | ||
| ASSERT_EQ(err, U_ILLEGAL_ARGUMENT_ERROR); | ||
| } | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way in CI to verify that we definitely don't have ICU in the binary somewhow? E.g. with readelf or the like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may not need to deal with ICU at all here. We just need the
ParseStandardURLfunction which does use or include anything from ICU.I think what would work is this:
ParseStandardURLfunction. I think this should not involve any files that reference ICU.Note that we may also need the
RemoveURLWhitespacefunction. I'm yet unsure if this function is needed. I'll look into it later tonight.Http::Utility::Urlclass to use theParseStandardURLfrom google URL.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yanavlasov sorry, a question. I can definitely pull only the
ParseStandardURLrelated files (that will beurl/third_party/mozilla/url_parse.{cc, h}) and let theHttp::Utility::Urlimplementation to use that, however, how about:envoy/source/common/http/path_utility.cc
Lines 3 to 4 in deded53
chromium_urlaround? cc. @htuch.If yes then we don't need to pull googleurl from googlesource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will removal
common/chromium_urlin favor of this eventually, but will need to keep it around while we do the runtime guarded transition.We should still verify ICU is not creeping into the build, since this is a regression check for the previous CVE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ultimately we want something to replace the uses in https://github.com/envoyproxy/envoy/tree/master/source/common/chromium_url, which are around
url::CanonicalizePath(). Can we do this without the ICU shim? What about pulling other URL parsing functionality out? Do we need build gymnastics to skip ICU in these cases, e.g. patch files?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. So from my understanding, the caller site is
envoy/source/common/http/path_utility.cc
Lines 15 to 28 in dd0befa
ParseStandardURL. I will try to look at this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a few attempts, I think we can't do a clean "import" e.g. referencing
third_party/mozilla/url_parse.ccfrom a BUILD file in envoy can cause a visibility problem (can be solved by usingexports_filesbut that requires a patch to upstream), without as @htuch said, a custom genrule_cmd.re: fuzzing, the motivation was to make sure if we have a googleurl lib linked with the shimmed ICU lib should not causing crashes (but yeah this should be predictable by inputting URLs with IDN to the function). If we want to skip testing it via GURL class, I can remove the test and do testing by calling "url::CanonicalizePath" instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a simple
exports_filespatch is OK. What we're really weary of is complex patches that are likely to break on version upgrades.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Submitted a draft here: #14583, patching the googleurl's url/BUILD file.