Skip to content

dns: fix callback contract bug in #4307.#4346

Closed
htuch wants to merge 1 commit intoenvoyproxy:masterfrom
htuch:resolver-teardown
Closed

dns: fix callback contract bug in #4307.#4346
htuch wants to merge 1 commit intoenvoyproxy:masterfrom
htuch:resolver-teardown

Conversation

@htuch
Copy link
Member

@htuch htuch commented Sep 5, 2018

Previously, once the callback was posted to the dispatcher, the
PendingResolution was destructed. This then broke the ability to
cancel() after the post. This PR restores this capability and simplifies
some of the object ownership aspects of PendingResolution post #4307.

Fixes oss-fuzz issue
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10184.

Risk level: Medium (this code has scary complicated lifetime and
ownership guarantees).
Testing: Additional unit test and corpus entry added.

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

Previously, once the callback was posted to the dispatcher, the
PendingResolution was destructed. This then broke the ability to
cancel() after the post. This PR restores this capability and simplifies
some of the object ownership aspects of PendingResolution post envoyproxy#4307.

Fixes oss-fuzz issue
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10184.

Risk level: Medium (this code has scary complicated lifetime and
  ownership guarantees).
Testing: Additional unit test and corpus entry added.

Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch
Copy link
Member Author

htuch commented Sep 5, 2018

@ggreenway can you take a look since you have the #4307 context? Thanks.

@ggreenway
Copy link
Member

My first thought, after glancing at the code: what about just reverting #4307 and wrapping the cares callback in a try/catch? That would be a low-risk change, and keep the code less complicated.

@htuch
Copy link
Member Author

htuch commented Sep 5, 2018

@ggreenway if we do that, how do we safely reject the configuration? Post the exception generation back to the main thread?

@ggreenway
Copy link
Member

Sorry, I didn't mean revert all of #4307. The part that does validation earlier should remain. The rest was just adding safety for any code that throws in the future, wasn't it? If so, we can keep the safety with a try/catch, and either log it or abort() if we feel it shouldn't ever happen, or post a throw to the main thread if we really want it to hit the top-level try/catch.

@htuch
Copy link
Member Author

htuch commented Sep 6, 2018

@ggreenway OK, I will close this out and I will switch to posting the exception only.

@htuch htuch closed this Sep 6, 2018
@htuch htuch deleted the resolver-teardown branch September 6, 2018 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants