Repository navigation
pjsip-test: don't destroy a TCP transport after releasing it - #5349
Conversation
|
The heap-use-after-free in |
| } | ||
|
|
||
| pjsip_transport_shutdown(tcp); | ||
| pjsip_transport_dec_ref(tcp); |
There was a problem hiding this comment.
Before, this transport was destroyed right away. Now it is destroyed later by the zero-delay timer, possibly on another thread, so it may still be alive (bound to the listener) during the second restart and the later lis_start, which isn't the state the test was written for. Could we add flush_events() after the release, as listener_destroy_local_name_test does?
There was a problem hiding this comment.
Fixed in 3a197cf. The transport is now gone before the second restart.
| /* Destroy this transport. */ | ||
| /* Once released, another thread may destroy it */ | ||
| pjsip_transport_shutdown(tcp[i]); | ||
| pjsip_transport_dec_ref(tcp[i]); |
There was a problem hiding this comment.
With the return -90 check gone, nothing verifies that these transports actually get destroyed. Maybe check pjsip_tpmgr_get_transport_count() after a flush_events() to keep that coverage?
There was a problem hiding this comment.
Fixed in 3a197cf. The test now fails if a released transport isn't destroyed. It counts destroys per transport, since the global count also changes with tests running in parallel.
transport_tcp_testreleases some transports and then destroys them. A released transport that is already shut down is destroyed at once by whichever thread polls the timers, so the test can destroy it a second time, or after it has been freed. Two of them are normally shut down at that point: the one inlistener_destroy_local_name_test, whose connect fails once the listener is gone, and the one to 1.1.1.1:80 at the end of the test, which the remote closes after a while.We hit it in local runs of pjsip-test with 3 workers in shuffled order: a heap-use-after-free when the idle timer destroys the transport on another test's thread, and once an assertion on the transport's destroyed lock.
Now the test shuts its transports down and releases them, as the documentation of
pjsip_transport_destroy()asks of applications. This includes the transport inrestart_failure_test, which isn't shut down when it is released, so it had no race.Processing the events between releasing and destroying the transport, as another thread would, makes the old test fail with a use-after-free at either of the two places. With this change the test passes, and it now also checks that the released transports are destroyed.
Tested on Linux with ASan.