listener: Fix incorrect error status handling when failing to change netns#40933
listener: Fix incorrect error status handling when failing to change netns#40933tonya11en merged 5 commits intoenvoyproxy:mainfrom
Conversation
When creating a listener socket in another network namespace, a nested `absl::StatusOr<>` is returned. The outer status shows the result of the attempt to switch network namespaces and the inner status is the result of the creation of a listener socket. This patch fixes a bug where we checked the inner status when we should have been checking the outer status. This results in a segfault because the outer status is dereferenced without first checking if it was OK. We now check the correct results and additional comments have been added to improve clarity of this code. Signed-off-by: Tony Allen <txallen@google.com>
Signed-off-by: Tony Allen <txallen@google.com>
adisuissa
left a comment
There was a problem hiding this comment.
Thanks!
Overall LGTM, modulo additional test.
Signed-off-by: Tony Allen <txallen@google.com>
Signed-off-by: Tony Allen <txallen@google.com>
adisuissa
left a comment
There was a problem hiding this comment.
LGTM, thanks!
Assigning a senior-maintainer as this touches core code.
/assign-from @envoyproxy/senior-maintainers
| } | ||
|
|
||
| TEST_F(ExecInNetnsTest, FailtoReturnToOriginalNetns) { | ||
| EXPECT_DEATH( |
There was a problem hiding this comment.
Somewhat orthogonal to this PR: I'm not very familiar with the internal design, but I suggest considering alternatives to death in some of these cases. I assume this is not happening after the config ingestion, and maybe one can mark a listener as broken/unhealthy instead of killing the entire proxy.
There was a problem hiding this comment.
This scenario is pretty bad, though. It means the actual OS thread is stuck in another network namespace and would need to be discarded. We would essentially lose the worker thread.
| // that wraps the result of the function we pass in, which is another `absl::StatusOr`. | ||
| auto outer_result = Network::Utility::execInNetworkNamespace(fn, netns.value().c_str()); | ||
|
|
||
| // We have a nested absl::StatusOr type. The "outer" result is the result of our attempt to jump |
There was a problem hiding this comment.
I guess this could've been:
RETURN_IF_NOT_OK_REF(outer_result.status());
return outer_result.value();
but I like the additional comments that explain the outer/inner relationship.
|
@envoyproxy/senior-maintainers assignee is @zuercher |
When creating a listener socket in another network namespace, a nested
absl::StatusOr<>is returned. The outer status shows the result of the attempt to switch network namespaces and the inner status is the result of the creation of a listener socket.This patch fixes a bug where we checked the inner status when we should have been checking the outer status. This results in a segfault because the outer status is dereferenced without first checking if it was OK.
We now check the correct results and additional comments have been added to improve clarity of this code.
Risk Level: Low
Testing: New unit test and fuzz test case
Docs Changes: n/a
Release Notes: Added
Platform Specific Features: Linux only