[core] Changed conditions for setting callbacks (#2683).

The conditions for `srt_listen_callback` and `srt_connect_callback` are that they can only be modified prior to listening or connecting, simultaneously they can be also set to NULL.
Haivision · Sep 19, 2023 · 83d91af · 83d91af
1 parent 4e8d036
commit 83d91af
Show file tree

Hide file tree

Showing 4 changed files with 17 additions and 11 deletions.
diff --git a/docs/API/API-functions.md b/docs/API/API-functions.md
@@ -752,7 +752,7 @@ connection has been accepted.
 **Arguments**:
 
 * `lsn`: Listening socket where you want to install the callback hook
-* `hook_fn`: The callback hook function pointer
+* `hook_fn`: The callback hook function pointer (or NULL to remove the callback)
 * `hook_opaque`: The pointer value that will be passed to the callback function
 
 |      Returns                  |                                                            |
@@ -763,7 +763,7 @@ connection has been accepted.
 
 |       Errors                      |                                           |
 |:--------------------------------- |:----------------------------------------- |
-| [`SRT_EINVPARAM`](#srt_einvparam) | Reported when `hook_fn` is a null pointer |
+| [`SRT_ECONNSOCK`](#srt_econnsock) | It can't be modified in a connected socket|
 | <img width=240px height=1px/>     | <img width=710px height=1px/>                      |
 
 The callback function has the signature as per this type definition:
@@ -1032,7 +1032,7 @@ connection failures.
 **Arguments**:
 
 * [`u`](#u): Socket or group that will be used for connecting and for which the hook is installed
-* `hook_fn`: The callback hook function pointer
+* `hook_fn`: The callback hook function pointer (or NULL to remove the callback)
 * `hook_opaque`: The pointer value that will be passed to the callback function
 
 
@@ -1044,7 +1044,7 @@ connection failures.
 
 |       Errors                       |                                           |
 |:---------------------------------- |:------------------------------------------|
-| [`SRT_EINVPARAM`](#srt_einvparam)  | Reported when `hook_fn` is a null pointer |
+| [`SRT_ECONNSOCK`](#srt_econnsock)  | It can't be modified in a connected socket|
 | <img width=240px height=1px/>      | <img width=710px height=1px/>                      |
 
 

diff --git a/srtcore/core.h b/srtcore/core.h
@@ -964,11 +964,17 @@ class CUDT
 private:
     void installAcceptHook(srt_listen_callback_fn* hook, void* opaq)
     {
+        if (m_bConnected || m_bConnecting || m_bListening || m_bBroken)
+            throw CUDTException(MJ_NOTSUP, MN_ISCONNECTED, 0);
+
         m_cbAcceptHook.set(opaq, hook);
     }
 
     void installConnectHook(srt_connect_callback_fn* hook, void* opaq)
     {
+        if (m_bConnected || m_bConnecting || m_bListening || m_bBroken)
+            throw CUDTException(MJ_NOTSUP, MN_ISCONNECTED, 0);
+
         m_cbConnectHook.set(opaq, hook);
     }
 

diff --git a/srtcore/srt_c_api.cpp b/srtcore/srt_c_api.cpp
@@ -387,17 +387,11 @@ int srt_setrejectreason(SRTSOCKET sock, int value)
 
 int srt_listen_callback(SRTSOCKET lsn, srt_listen_callback_fn* hook, void* opaq)
 {
-    if (!hook)
-        return CUDT::APIError(MJ_NOTSUP, MN_INVAL);
-
     return CUDT::installAcceptHook(lsn, hook, opaq);
 }
 
 int srt_connect_callback(SRTSOCKET lsn, srt_connect_callback_fn* hook, void* opaq)
 {
-    if (!hook)
-        return CUDT::APIError(MJ_NOTSUP, MN_INVAL);
-
     return CUDT::installConnectHook(lsn, hook, opaq);
 }
 

diff --git a/test/test_listen_callback.cpp b/test/test_listen_callback.cpp
@@ -48,9 +48,15 @@ class ListenerCallback
         ASSERT_EQ(inet_pton(AF_INET, "127.0.0.1", &bind_sa.sin_addr), 1);
         bind_sa.sin_port = htons(5555);
 
+        // NOTE: listener callback must be set BEFORE LISTENING,
+        // otherwise it may potentially change the procedure in the middle of processing
+        // of an incoming connection.
+        ASSERT_NE(srt_listen_callback(server_sock, &SrtTestListenCallback, NULL), -1);
         ASSERT_NE(srt_bind(server_sock, (sockaddr*)&bind_sa, sizeof bind_sa), -1);
         ASSERT_NE(srt_listen(server_sock, 5), -1);
-        (void)srt_listen_callback(server_sock, &SrtTestListenCallback, NULL);
+
+        // Check also changing the listener callback AFTER listening - this is expected to fail.
+        ASSERT_EQ(srt_listen_callback(server_sock, &SrtTestListenCallback, NULL), -1);
 
         accept_thread = std::thread([this] { this->AcceptLoop(); });