build: update c-ares to 1.16.1. by htuch · Pull Request #11149 · envoyproxy/envoy

htuch · 2020-05-11T18:20:29Z

This picks up ares_getaddrinfo() and also some security fixes.

See https://c-ares.haxx.se/changelog.html#1_16_1.

Risk level: Low
Testing: bazel test //test/...

Signed-off-by: Harvey Tuch htuch@google.com

This picks up ares_getaddrinfo() and also some security fixes. See https://c-ares.haxx.se/changelog.html#1_16_1. Risk level: Low Testing: bazel test //test/... Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2020-05-11T18:21:25Z

CC @moderation @wrowe (I removed the Windows patch as it was broken, will reintroduce as CI dictates).

junr03 · 2020-05-11T18:57:48Z

Prevent possible use-after-free and double-free in ares_getaddrinfo() if ares_destroy() is called prior to ares_getaddrinfo() completing.

Nice, they fixed the bug our ASAN build uncovered!

Signed-off-by: Harvey Tuch <htuch@google.com>

bazel/envoy_binary.bzl

Signed-off-by: Harvey Tuch <htuch@google.com>

wrowe · 2020-05-12T18:32:28Z

bazel/repositories.bzl

    location = _get_location("com_github_c_ares_c_ares")
    http_archive(
        name = "com_github_c_ares_c_ares",
-        patches = ["@envoy//bazel/foreign_cc:cares-win32-nameser.patch"],


Question, why was this dropped? The //source/exe:envoy-static doesn't require this patch, however, the //test/common/network:dns_impl_test does need this hack to provide windows a nameser.h (which isn't in the windows API at all, that's why c-ares bundles the file.)

@wrowe is this in the tests that are exercised by CI right now? The Windows job is passing. I dropped the patch as I wasn't sure it was needed and didn't want to do the work of updating if not; in general these patches are going to impact our ability to upgrade versions if they aren't upstreamed.

Correct, right now we don't build tests on Windows, because the Azure workers don't have sufficient memory to compile much of anything with respect to the mocks (they eat up about 4GB of heap when included.) But it is necessary to retain that patch, and it won't be absorbed upstream because of what we are accomplishing with it. We are "borrowing" their nameser.h, because it is the most convenient way to obtain it.

(Sunjay and I are building the tests frequently, and they are going live with the move to the RBE picture for Windows builds.)

I think there might be other ways to get this header, e.g. via a genrule that extracts it from the http_archive. Or maybe even directly via a repository rule. I'd be happy to work on that in this PR if there was a way to validate it. Is there a way to enable this test?

I'm very much opposed to permanently maintaining cmake patches, since these are fragile and will impede our ability to track security updates by imposing a higher cost on folks making these updates.

While I don't disagree with you, we can simply disable the test with tags = ["skip_on_windows"] and just forget about it. Looking at the situation, this seemed at the time to be the most viable.

Signed-off-by: Harvey Tuch <htuch@google.com>

wrowe

No, please do not do this. The global tags the build observes are "skip_on_windows" (total compile failure) and "fails_on_windows" (won't pass and known issue.) If we did this command-line, it would be ridonculous at this point in time. Just tweak the BUILD file to tags= out the test.

Signed-off-by: Harvey Tuch <htuch@google.com>

sunjayBhatia · 2020-05-14T15:12:59Z

@htuch you should be able to apply this (and undo all other changes to work around the nameser.h thing for Windows) and it should work, probably what we shouldve done in the first place instead of a patch

diff --git a/bazel/foreign_cc/BUILD b/bazel/foreign_cc/BUILD
index 134b9d470..c2f558a87 100644
--- a/bazel/foreign_cc/BUILD
+++ b/bazel/foreign_cc/BUILD
@@ -87,6 +87,10 @@ envoy_cmake_external(
         "//bazel:apple": ["-lresolv"],
         "//conditions:default": [],
     }),
+    postfix_script = select({
+        "//bazel:windows_x86_64": "cp -L $EXT_BUILD_ROOT/external/com_github_c_ares_c_ares/nameser.h $INSTALLDIR/include/nameser.h",
+        "//conditions:default": "",
+    }),
     static_libraries = select({
         "//bazel:windows_x86_64": ["cares.lib"],
         "//conditions:default": ["libcares.a"],

htuch · 2020-05-14T20:10:52Z

@sunjayBhatia ack, thanks for the suggestion.

Signed-off-by: Harvey Tuch <htuch@google.com>

moderation · 2020-05-15T21:15:54Z

Looks like this change has broken compilation on RHEL 7. Bazel is 3.1.0 and building with bazel build -c opt --copt=-DENVOY_IGNORE_GLIBCXX_USE_CXX11_ABI_ERROR=1 //source/exe:envoy-static.stripped. /cc @htuch @lizan

INFO: From CcCmakeMakeRule bazel/foreign_cc/ares/include:

ERROR: /envoyproxy/envoy/bazel/foreign_cc/BUILD:79:1: output 'bazel/foreign_cc/ares/lib/libcares.a' was not created
ERROR: /envoyproxy/envoy/bazel/foreign_cc/BUILD:79:1: not all outputs were created or valid
Target //source/exe:envoy-static.stripped failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 953.248s, Critical Path: 59.32s
INFO: 418 processes: 418 linux-sandbox.
FAILED: Build did NOT complete successfully

#11149 broke CentOS/RHEL build, force lib install dir to lib rather than lib64 in those distro, so rules_foreign_cc can find built static libraries. Signed-off-by: Lizan Zhou <lizan@tetrate.io>

This picks up ares_getaddrinfo() and also some security fixes. See https://c-ares.haxx.se/changelog.html#1_16_1. Risk level: Low Testing: bazel test //test/... Signed-off-by: Harvey Tuch <htuch@google.com>

envoyproxy#11149 broke CentOS/RHEL build, force lib install dir to lib rather than lib64 in those distro, so rules_foreign_cc can find built static libraries. Signed-off-by: Lizan Zhou <lizan@tetrate.io>

build: update c-ares to 1.16.1.

67dbaad

This picks up ares_getaddrinfo() and also some security fixes. See https://c-ares.haxx.se/changelog.html#1_16_1. Risk level: Low Testing: bazel test //test/... Signed-off-by: Harvey Tuch <htuch@google.com>

htuch requested a review from lizan May 11, 2020 18:20

htuch assigned lizan May 11, 2020

lizan previously approved these changes May 11, 2020

View reviewed changes

Mac OS X needs libresolv.

c91e036

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch dismissed lizan’s stale review via c91e036 May 11, 2020 19:25

libresolv in tests as well.

0f74a84

Signed-off-by: Harvey Tuch <htuch@google.com>

lizan reviewed May 11, 2020

View reviewed changes

bazel/envoy_binary.bzl Outdated Show resolved Hide resolved

htuch added 2 commits May 12, 2020 08:35

Move libresolv to cmake linkopt.

be5f186

Signed-off-by: Harvey Tuch <htuch@google.com>

fix_format

f62eb96

Signed-off-by: Harvey Tuch <htuch@google.com>

wrowe suggested changes May 12, 2020

View reviewed changes

Temporary hack to get dns_impl_test to run on Windows CI (maybe?)

9330749

Signed-off-by: Harvey Tuch <htuch@google.com>

wrowe reviewed May 12, 2020

View reviewed changes

htuch added 2 commits May 12, 2020 20:27

Experment with extracting nameser.h out of http_archive.

c468a02

Signed-off-by: Harvey Tuch <htuch@google.com>

Select nameser.h only on Windows.

464b1ad

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch added 2 commits May 14, 2020 16:11

Merge remote-tracking branch 'upstream/master' into c-ares-1-16-1

1716ba0

@sunjayBhatia suggested postfix_script.

979c79a

Signed-off-by: Harvey Tuch <htuch@google.com>

lizan approved these changes May 15, 2020

View reviewed changes

lizan merged commit 3cd4a0f into envoyproxy:master May 15, 2020

htuch deleted the c-ares-1-16-1 branch May 15, 2020 01:35

lizan mentioned this pull request May 16, 2020

build: fix cares build #11225

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: update c-ares to 1.16.1.#11149

build: update c-ares to 1.16.1.#11149
lizan merged 10 commits intoenvoyproxy:masterfrom
htuch:c-ares-1-16-1

htuch commented May 11, 2020

Uh oh!

htuch commented May 11, 2020

Uh oh!

junr03 commented May 11, 2020

Uh oh!

Uh oh!

wrowe May 12, 2020

Uh oh!

htuch May 12, 2020

Uh oh!

wrowe May 12, 2020

Uh oh!

wrowe May 12, 2020

Uh oh!

htuch May 12, 2020

Uh oh!

wrowe May 12, 2020

Uh oh!

wrowe left a comment

Uh oh!

sunjayBhatia commented May 14, 2020

Uh oh!

htuch commented May 14, 2020

Uh oh!

moderation commented May 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

htuch commented May 11, 2020

Uh oh!

htuch commented May 11, 2020

Uh oh!

junr03 commented May 11, 2020

Uh oh!

Uh oh!

wrowe May 12, 2020

Choose a reason for hiding this comment

Uh oh!

htuch May 12, 2020

Choose a reason for hiding this comment

Uh oh!

wrowe May 12, 2020

Choose a reason for hiding this comment

Uh oh!

wrowe May 12, 2020

Choose a reason for hiding this comment

Uh oh!

htuch May 12, 2020

Choose a reason for hiding this comment

Uh oh!

wrowe May 12, 2020

Choose a reason for hiding this comment

Uh oh!

wrowe left a comment

Choose a reason for hiding this comment

Uh oh!

sunjayBhatia commented May 14, 2020

Uh oh!

htuch commented May 14, 2020

Uh oh!

moderation commented May 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants