test: fix use-after-free in fake upstream#14067
test: fix use-after-free in fake upstream#14067yanavlasov merged 4 commits intoenvoyproxy:masterfrom
Conversation
Fix a use-after-free that causes rare (1%) flakes of //test/integration:tcp_tunneling_integration_test. The ReadFilter gets registered with the connection but shouldn't call into its parent after the parent has been deleted. This can be done by using a weak_ptr proxy to the parent. Signed-off-by: Alex Konradi <akonradi@google.com>
akonradi
changed the title
|
Seems like some clean-up order issue. I would think that FakeConnection should always be destroyed before the Network::Connection |
|
That's exactly the issue. FakeRawConnection gets destroyed before the ReadFilter object it registers with the Network::Connection, resulting in the use-after-free |
Can't the FakeRawConnection call removeReadFilter before it is destroyed? I would rather figure lifecycle management of these objects that create a shared_ptr to this as it is super hacky. |
Signed-off-by: Alex Konradi <akonradi@google.com>
|
Thanks, didn't know that was a thing. That's a whole lot cleaner than the weak proxy. |
Signed-off-by: Alex Konradi <akonradi@google.com>
|
It looks like there were some asan and tsan failures in a different test, though related to this change. I'm currently debugging. |
This allows the SharedConnectionWrapper to be delted first or second. If the wrapper is deleted first, the read filter will be deleted and the FakeHttpConnection will not attempt to remove it. If the FakeHttpConnection is deleted first, it will remove the filter from the SharedConnectionWrapper, preventing the original use-after-free. Signed-off-by: Alex Konradi <akonradi@google.com>
|
Okay, fixed the failures with a different weak_ptr. This is much less weird than the originally proposed change. |
Fix a test-only use-after-free that causes flakes of //test/integration:tcp_tunneling_integration_test. Signed-off-by: Alex Konradi <akonradi@google.com> Signed-off-by: Qin Qin <qqin@google.com>
2 participants
Commit Message: Fix a test-only use-after-free that causes flakes of //test/integration:tcp_tunneling_integration_test.
Additional Description:
The bug causes rare flakes of tcp_tunneling_integration_test, on the order of 0.5-1% under msan. The ReadFilter gets registered with the connection but shouldn't call into its parent after the parent has been deleted. This is done by using a weak_ptr proxy to the parent.
Risk Level: low
Testing: ran affected test
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a