[utility]: wrap try in an assertion to ensure that exception are only caught on main thread by chaoqin-li1123 · Pull Request #15251 · envoyproxy/envoy

chaoqin-li1123 · 2021-03-02T01:06:59Z

Commit Message: Define a macros that wrap try with an assertion that exceptions are only caught in main thread. Currently, envoy use c++ exception for error propagation. This PR is one of the steps to address #14320. The long term goal is to disallow raw try in envoy core code and eliminate c++ exception from data plane, which can improve exception safety. The try in the PR happen on main thread and can be wrapped in main thread assertion without breaking any existing test. In the following PR, raw try that can not be replaced by TRY_ASSERT_MAIN_THREAD will be removed from core codebase with other error propagation.
Additional Description:
Risk Level:
Testing: none
Docs Changes: none
Release Notes: none
Platform Specific Features: none
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

alyssawilk · 2021-03-02T14:31:22Z

cc @mattklein123 for thoughts.

Also please ignore the mac CI timeout - we're looking into it!

mattklein123

At a high level this makes sense and looks great to me. Are you going to add a linter also?

/wait

chaoqin-li1123 · 2021-03-02T17:49:26Z

At a high level this makes sense and looks great to me. Are you going to add a linter also?

/wait

I plan to add format checking later, which disallow "try {" in /source repo but whitelist the /source/extensions repo. But that will be after all the raw try has been removed from core codebase of envoy.

mattklein123 · 2021-03-02T18:35:30Z

I plan to add format checking later, which disallow "try {" in /source repo but whitelist the /source/extensions repo. But that will be after all the raw try has been removed from core codebase of envoy.

My concern is that we may play whackamole until then. What about a TRY_NEEDS_AUDIT macro and then we can lint for try now?

chaoqin-li1123 · 2021-03-02T19:55:32Z

I plan to add format checking later, which disallow "try {" in /source repo but whitelist the /source/extensions repo. But that will be after all the raw try has been removed from core codebase of envoy.

My concern is that we may play whackamole until then. What about a TRY_NEEDS_AUDIT macro and then we can lint for try now?

Yes, that makes sense. I will add format checking now with this workaround.

htuch · 2021-03-03T02:49:50Z

+1 on linting in CI. Thanks @chaoqin-li1123 this looks great.

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

…ith_assert_macros

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

…ith_assert_macros

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

mattklein123

Thanks LGTM with some small comments.

/wait

source/common/common/regex.cc

source/common/common/thread.h

mattklein123 · 2021-03-04T01:08:04Z

source/common/formatter/substitution_formatter.cc

-  try {
-    MessageUtil::jsonConvertValue(*proto, val);
-  } catch (EnvoyException& ex) {
+  TRY_ASSERT_MAIN_THREAD { MessageUtil::jsonConvertValue(*proto, val); }


Pretty sure this is on workers?

Doesn't break any test when I add the main thread assertion. maybe because of missing test coverage, I will take a look at the test coverage later.

Ping please audit this. I'm 100% positive this happens on workers given the function params. (This feature should probably have an integration test anyway so maybe add that in a different PR?)

Thanks, I will see whether some integration test can be added.

source/common/http/utility.cc

source/common/network/dns_impl.cc

source/common/protobuf/utility.cc

source/server/admin/utils.cc

tools/code_format/check_format.py

jmarantz

just 2 remaining nits from my perspective.

thanks for pushing this through!

test/common/formatter/substitution_formatter_test.cc

source/common/formatter/substitution_formatter.cc

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

mattklein123

Thanks LGTM with small comments, thanks!

/wait

source/common/http/utility.cc

mattklein123 · 2021-03-25T18:09:11Z

source/common/network/dns_impl.cc


  if (completed_) {
    if (!cancelled_) {
+      // TODO(chaoqin-li1123): remove this exception catching since the process is dying anyway.


This comment is not accurate. This can happen during normal operation for some of these I think. Doesn't this need some AUDIT statement or CI should fail? Same below?

Thanks, will update it.

This file is in RAW_TRY_ALLOWLIST in check_format.py. However I'm not sure if this is justified and think maybe it should be a NEEDS_AUDIT.

Do we need to allow the callback to throw? Or can we just have the callbacks return an error?

I assume it isn't c_ares that forces us to handle exceptions here?

This code is executed in main thread and in dns filter, I think it is fine to allow a filter to throw and catch.

Thanks, comment updated.

source/common/network/io_socket_handle_impl.cc

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

chaoqin-li1123 · 2021-03-26T22:53:59Z

/retest

repokitteh-read-only · 2021-03-26T22:54:03Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15251 (comment) was created by @chaoqin-li1123.

see: more, trace.

jmarantz

@mattklein123 this PR will not close the Issue -- it's just the first step. Then we have to go through and fix all the audits. But getting the mechanism in place is a great stepping stone.

Matt -- you approved, but I wanted to double check you are you OK to merge? There were a couple of unanswered comment threads I think.

source/common/http/utility.cc

mattklein123 · 2021-03-29T00:32:18Z

I'm fine to merge as long as we have a good plan for all the follow ups. @jmarantz I will defer to you for final review/merge, thanks!

chaoqin-li1123 · 2021-03-29T03:39:53Z

There are only 4 TRY_NEEDS_AUDIT left, I think I can clean them up in the followup PRs.

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

jmarantz

Just a couple of unresolved comments and then we can merge.

jmarantz · 2021-03-29T12:23:05Z

source/common/network/dns_impl.cc


  if (completed_) {
    if (!cancelled_) {
+      // TODO(chaoqin-li1123): remove this exception catching since the process is dying anyway.


This file is in RAW_TRY_ALLOWLIST in check_format.py. However I'm not sure if this is justified and think maybe it should be a NEEDS_AUDIT.

Do we need to allow the callback to throw? Or can we just have the callbacks return an error?

I assume it isn't c_ares that forces us to handle exceptions here?

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

source/common/network/dns_impl.cc

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

source/common/network/io_socket_handle_impl.cc

chaoqin-li1123 · 2021-03-30T06:09:37Z

/retest

repokitteh-read-only · 2021-03-30T06:09:41Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15251 (comment) was created by @chaoqin-li1123.

see: more, trace.

chaoqin-li1123 added 11 commits February 23, 2021 16:42

initial

d33a185

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

fix format

6ec2e5f

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

remove some raw try and fix format

626b800

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

access log

b053baf

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

fix test

5d90db0

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

new macros

f4e4c33

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

USE new macros

5be7631

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

fix format

36d33b9

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

ASSERT

2c62c2f

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

change macros name

e5aae22

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

change macros name

5d7fbf3

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

chaoqin-li1123 requested a review from htuch as a code owner March 2, 2021 01:07

alyssawilk assigned yanavlasov Mar 2, 2021

mattklein123 self-assigned this Mar 2, 2021

mattklein123 requested changes Mar 2, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 2, 2021

add format checking

7bbfe9e

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

repokitteh-read-only bot removed the waiting label Mar 3, 2021

chaoqin-li1123 added 5 commits March 3, 2021 15:39

Merge branch 'main' of https://github.com/envoyproxy/envoy into try_w…

d5ed721

…ith_assert_macros

Merge branch 'main' of https://github.com/envoyproxy/envoy into try_w…

75fba5d

…ith_assert_macros

sync

e600c73

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into try_w…

3bd5785

…ith_assert_macros

fix format

e4b5d8a

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

mattklein123 requested changes Mar 4, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 4, 2021

repokitteh-read-only bot removed the waiting label Mar 25, 2021

jmarantz reviewed Mar 25, 2021

View reviewed changes

test/common/formatter/substitution_formatter_test.cc Outdated Show resolved Hide resolved

source/common/formatter/substitution_formatter.cc Show resolved Hide resolved

chaoqin-li1123 added 3 commits March 25, 2021 05:23

fix format

b20f133

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

clean up

fab050c

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

add test coverage

d14310d

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

mattklein123 requested changes Mar 25, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 25, 2021

fix comment

b32053a

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

repokitteh-read-only bot removed the waiting label Mar 25, 2021

mattklein123 previously approved these changes Mar 26, 2021

View reviewed changes

repokitteh-read-only bot added the waiting:any label Mar 28, 2021

jmarantz reviewed Mar 28, 2021

View reviewed changes

source/common/http/utility.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot removed the waiting:any label Mar 28, 2021

clean up

9ed6baa

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

chaoqin-li1123 dismissed mattklein123’s stale review via 9ed6baa March 29, 2021 03:51

jmarantz reviewed Mar 29, 2021

View reviewed changes

jmarantz mentioned this pull request Mar 29, 2021

http: make HeaderHashMethod hashing all the header values #15486

Merged

mattklein123 removed their assignment Mar 29, 2021

fix comment

a2716ac

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

jmarantz reviewed Mar 29, 2021

View reviewed changes

source/common/network/dns_impl.cc Outdated Show resolved Hide resolved

fix comment

afc6e1d

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

jmarantz approved these changes Mar 29, 2021

View reviewed changes

source/common/network/io_socket_handle_impl.cc Show resolved Hide resolved

jmarantz approved these changes Mar 30, 2021

View reviewed changes

jmarantz merged commit cfbb1a8 into envoyproxy:main Mar 30, 2021

Conversation

chaoqin-li1123 commented Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk commented Mar 2, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

chaoqin-li1123 commented Mar 2, 2021

Uh oh!

mattklein123 commented Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chaoqin-li1123 commented Mar 2, 2021

Uh oh!

htuch commented Mar 3, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

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!

Uh oh!

chaoqin-li1123 commented Mar 26, 2021

Uh oh!

repokitteh-read-only bot commented Mar 26, 2021

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 commented Mar 29, 2021

Uh oh!

chaoqin-li1123 commented Mar 29, 2021

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chaoqin-li1123 commented Mar 30, 2021

Uh oh!

repokitteh-read-only bot commented Mar 30, 2021

Uh oh!

Reviewers

chaoqin-li1123 commented Mar 2, 2021 •

edited

Loading

mattklein123 commented Mar 2, 2021 •

edited

Loading