-
Notifications
You must be signed in to change notification settings - Fork 280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Send callback is invoked on already closed socket. #405
Comments
I've finally completed bisect to find out the commit which introduced invocation of send callback on closed socket. view bisect.log (can be replayed with `git bisect replay < bisect.log`)
|
This is still a problem for us. @tuexen, do you have any plan how to deal with this? Is there something we should be doing at the client level to wait for the socket to really be closed? Otherwise it seems like Would that pose a problem though? It would mean that when this happens, we wouldn't send out the abort chunk (that's what usrsctp is trying to do when the crash occurs). I guess delivery of that isn't guaranteed anyway though. |
So what is exactly the problem? When the user call |
Yes, the naïve assumption was that after calling And no, we're not using the upcall API. Should we be? Can you summarize how it works? This is what we're running into:
Presumably this is from the INPKILL timer. |
Any updates on this? I'm experiencing the same behavior and I don't know how to handle it. Are callbacks really supposed to still be called after close? If so, how do you know when it is safe to assume no more calls? |
I just realise: You are not referring to the send callback in the socket API (upper layer interface), but to the function to send out packets (lower layer interface). Doesn't deregistering the address do what you want? |
Same here, it's indeed about the lower layer function to send out packets (the one passed to On my side the callback was sometimes called after |
Let me double check... |
Thanks. Actually the call might not be technically posterior to deregistering but simply concurrent, which results in the same issue depending on thread scheduling. |
This patch tries to fix the problem of usrsctp calling callbacks even after the usrsctp_close function is invoked. Usrsctp seems to keep the asociation around until the teardown procedure is completed. The problem is that sometimes it is not possible to complete the shutdown procedure if the lower transport is gone like SCTP running on top of DTLS. In such case, callbacks triggered from usrsctp are providing an pointer address to an applications which could not be valid or dealocated. To let application know when they can safetely deallocate memory registered with an association, we are storing a destroy function which will be used for usrsctp to notify application when the association is gone. Related issues: sctplab#405 sctplab#147
This patch tries to fix the problem of usrsctp calling callbacks even after the usrsctp_close function is invoked. Usrsctp seems to keep the asociation around until the teardown procedure is completed. The problem is that sometimes it is not possible to complete the shutdown procedure if the lower transport is gone like SCTP running on top of DTLS. In such case, callbacks triggered from usrsctp are providing an pointer address to an applications which could have been deallocated. To let applications know when they can safetely deallocate memory registered with an association, we are storing a destroy function which will be used for usrsctp to notify applications when the association is gone. Related issues: sctplab#405 sctplab#147
This patch tries to fix the problem of usrsctp calling callbacks even after the usrsctp_close function is invoked. Usrsctp seems to keep the asociation around until the teardown procedure is completed. The problem is that sometimes it is not possible to complete the shutdown procedure if the lower transport is gone like SCTP running on top of DTLS. In such case, callbacks triggered from usrsctp are providing an pointer address to an applications which could have been deallocated. To let applications know when they can safely deallocate memory registered with an association, we are storing a destroy function which will be used for usrsctp to notify applications when the association is gone. Related issues: sctplab#405 sctplab#147
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread.
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread.
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread.
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread. (cherry picked from commit 9cf8ac2)
This patch tries to fix the problem of usrsctp calling callbacks even after the usrsctp_close function is invoked. Usrsctp seems to keep the asociation around until the teardown procedure is completed. The problem is that sometimes it is not possible to complete the shutdown procedure if the lower transport is gone like SCTP running on top of DTLS. In such case, callbacks triggered from usrsctp are providing an pointer address to an applications which could have been deallocated. To let applications know when they can safely deallocate memory registered with an association, we are storing a destroy function which will be used for usrsctp to notify applications when the association is gone. Related issues: sctplab/usrsctp#405 sctplab/usrsctp#147 From: sctplab/usrsctp#535
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread. (cherry picked from commit 9cf8ac2)
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread. (cherry picked from commit 9cf8ac2)
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread. (cherry picked from commit 9cf8ac2)
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread. (cherry picked from commit 9cf8ac2)
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread. (cherry picked from commit 9cf8ac2)
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread. (cherry picked from commit 9cf8ac2)
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread. (cherry picked from commit 9cf8ac2)
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread. (cherry picked from commit 9cf8ac2)
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread. (cherry picked from commit 9cf8ac2)
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread. (cherry picked from commit 9cf8ac2)
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread. (cherry picked from commit 9cf8ac2)
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread. (cherry picked from commit 9cf8ac2)
Workaround for sctplab/usrsctp#405: - Since the sctp socket can outlive the sctp assoacition, we need to only register/deregister the association when creating/closing the socket. This prevents invalid calls to sctp_packet_out() and receive_cb() in potential invalid states. Workaround for sctplab/usrsctp#383: - Retry usrsctp_finish() for 5seconds. This fixes a race condition between usrsctp_close() and usrsctp_finish() in which a dead socket will be accessed by the SCTP thread. (cherry picked from commit 9cf8ac2)
Summary:
usrsctp
invokes send callback on socket, which was already closed by user's code.Typically user code in send callback does some memory access to already freed memory.
This happens due to user's code assumption that send callback is never invoked after socket is closed with
usrsctp_close
.Reproduces:
Sometimes
Versions:
I've detected that bug happen on current master, but it was not presented on version used by Chromium: 7a8bc9a from 22 Jun 2018The first commit with the issue is 6bafb12, merged in master as part of PR #249 on 9 Aug 2018.
Steps to reproduce (abstract):
usrsctp
socket withSO_LINGER
set in non-blocking mode with send callback provided by user's code;usrsctp
socket withusrsctp_close
and release associated resources in user code;Expected result:
usrsctp
will never invoke send callback which is provided by user's code during socket initialization.Actual result:
Send callback is invoked for socket, which was previously closed with
usrsctp_close
. It causes user's code misbehavior, typically accessing already freed memory, causing crash due toSIGSEGV
.Steps to reproduce (concrete)
WebRTC
has gtest which is accidentally able to reproduce send callback invocation on closed socket.fetch webrtc
- fetchesWebRTC
source code;cd src
- rest of commands assumed to be executed in WebRTC source code directory;gclient sync
- checkout dependencies;gn gen out/ReleaseWithAsan --args='is_debug=false rtc_enable_sctp=true symbol_level=2 dcheck_always_on=true is_asan=true' --cflags="-fno-omit-frame-pointer -fno-sanitize-recover=all -fsanitize-address-use-after-scope"
- generate build files with optimizations and address sanitizer enabled;autoninja -C out/ReleaseWithAsan rtc_media_unittests
- compile unit tests;export ASAN_OPTIONS="handle_segv=1:detect_leaks=0:disable_coredump=0:unmap_shadow_on_exit=1:abort_on_error=1"
configure ASAN options;export ASAN_SYMBOLIZER_PATH="$(pwd)/third_party/llvm-build/Release+Asserts/bin/llvm-symbolizer"
configure ASAN symbolizer;gdb --args ./out/ReleaseWithAsan/rtc_media_unittests --gtest_filter="*AllMessagesAreDeliveredOverLossyConnectionConcurrentTests" --gtest_also_run_disabled_tests
- execute unittest with gdb attached.Investigation details:
It is occurred that
usrsctp_close
might not dispose/free socket.usrsctp_close
only remove reference from "user's" code, basically doing decrement of referene counter.And if there are any references to
struct socket *
the socket will surviveusrsctp_close
invocation:usrsctp/usrsctplib/user_socket.c
Lines 2192 to 2194 in 09768bf
where
sorele
:usrsctp/usrsctplib/user_socketvar.h
Lines 485 to 495 in 09768bf
so,
sofree
is only invoked when last reference removed.I've verified this by locally modifying
usrsctp_close
to have this:And I've got this assert triggered:
GDB backtrace of assert
For example,
WebRTC
assumes there will no calls fromusrsctp
toWebRTC
:snipped from WebRTC source
with the assumption repeated here:
additional snipped from WebRTC source
Below the typical crash backtrace I've observed when send callback was invoked on already closed socket:
typical backtrace from GDB of crashed application
I've checked several crash dumps when send callback is invoked.
In all observed cases
sctp_handle_tick
triggered execution ofSCTP_TIMER_TYPE_HEARTBEAT
timer, which in it's callback decided to close socket and invoke send callback:output from GDB
ASAN report on revision 6bafb12:
typical ASAN report produced on crash
There are several open question here:
usrsctp_close
completed execution?0
at the end ofusrsctp_close
invocation, causing actual socket release?The text was updated successfully, but these errors were encountered: