feat: add callback for unhandled STUN requests #1211

achingbrain · 2024-06-14T12:42:25Z

Calls the functions added to libjuice in paullouisageneau/libjuice#248 (this needs to be merged & released before CI will pass here).

Exports a OnUnhandledStunRequest function that can be passed a callback that will be invoked when an incoming STUN message is received that has no corresponding agent for the ICE ufrag.

Refs #1166

achingbrain · 2024-06-14T12:42:34Z

When implementing this I hadn't realised that libjuice will only ever allow one UDP socket to be used so unregistering the handler takes a host/port pair in order to unregister the handler only for that pair.

I hope this can be worked around as it's quite common to create two listeners during a test run and they'll not be able to co-exist in the same process like this.

We could, for example invalidate the module cache when loading the native part of node-datachannel to get a separate copy of libdatachannel that would run in it's own memory space but it's a bit of a hack.

Also if the application has already created a peer connection outside of libp2p's control, setting the host/port will have no effect as it would have already opened the port specified in the first invocation, and now we'd be setting the handler for that port which may not be what the user expects. Clarified by @xicilion below

Anyway, if you'd prefer a separate function without the host/port to remove the handler please shout.

It may not need the void *user_ptr arg to juice_bind_stun then since it can just invoke the global reference stored in unboundStunCallback.

xicilion · 2024-06-14T14:28:08Z

Also if the application has already created a peer connection outside of libp2p's control, setting the host/port will have no effect as it would have already opened the port specified in the first invocation, and now we'd be setting the handler for that port which may not be what the user expects.

This should not happen. In order to avoid such hidden errors, I added a check in juice_bind_stun to prevent binding when mux connections are detected, so as to avoid binding to ports that users do not expect.

xicilion · 2024-06-14T14:32:27Z

The Mux mode is bound to a unique socket, which has no problem at all in a normal WebRTC application, after all, you can't even set the port in the browser.

But in the Peer B application of WebRTC Direct, it is restricted by this. One process can only bind to one port, which greatly limits its usage.

Calls the functions added to libjuice in paullouisageneau/libjuice#248 Exports a `OnUnhandledStunRequest` function that can be passed a callback that will be invoked when an incoming STUN message is received that has no corresponding agent for the ICE ufrag. Closes paullouisageneau#1166

achingbrain · 2025-01-16T18:10:57Z

Needs the changes in paullouisageneau/libjuice#292 before this can be merged.

paullouisageneau · 2025-01-17T12:18:30Z

src/global.cpp

+
+#endif
+
+void OnUnhandledStunRequest ([[maybe_unused]] std::string host, [[maybe_unused]] int port, [[maybe_unused]] UnhandledStunRequestCallback callback, [[maybe_unused]] void *userPtr) {


The name should reflect that this listens only on mux mode, and that it attempts to listen to a specific port, you could rename it to ListenIceUdpMux() for instance, since the feature is called enableIceUdpMux in rtc::Configuration.

host should also be optional to allow any address (it would pass NULL to libjuice), and you could remame it to bindAddress as it is the name in the configuration. If it's not part of the mapping, I think it should be moved at the end and made an optional parameter.

paullouisageneau · 2025-01-17T12:27:41Z

src/global.cpp

+	if (callback == NULL) {
+		PLOG_DEBUG << "Removing unhandled STUN request listener";
+
+		free(unboundStunCallbacks.at(port));


This causes a race condition: If the callback is called in parallel it will cause a use after free.

Also, if no callback is set for the port it will throw an std::out_of_range exception which is confusing for the user so you should use find instead.

I've removed the map - ListenIceUdpMux now accepts a pointer to a callback and the caller of ListenIceUdpMux is responsible for it's memory management.

paullouisageneau · 2025-01-17T12:31:14Z

include/rtc/global.hpp

+	uint16_t remotePort;
+};
+
+RTC_CPP_EXPORT typedef std::function<void(UnhandledStunRequest request, void* userPtr)> UnhandledStunRequestCallback;


An std::function should not have a user pointer as it has its own context. The user can use bind, lambda capture, or a callable object.

Additionally, all callbacks in the library should be implemented with synchronized_callback to prevent race conditions between calling and setting/resetting.

Fixed the user pointer part.

I'm not sure I follow about synchronized_callback - where it's used elsewhere here a callback has been passed in that's stored as a class member. That member can get overwritten so I can see why we need to synchronize on it's access.

Here we're passing a reference to a function to libjuice and all state is passed along with the function invocation - do we still need synchronized_callback?

paullouisageneau · 2025-01-17T12:33:25Z

include/rtc/global.hpp

+struct UnhandledStunRequestHandler {
+	UnhandledStunRequestCallback callback;
+	void *userPtr;
+};


I think you don't need this since the user pointer is useless in C++.

paullouisageneau · 2025-01-17T12:34:54Z

include/rtc/rtc.h

+	uint16_t port;
+} rtcUnhandledStunRequest;
+
+typedef void(RTC_API *rtcUnhandledStunRequestCallbackFunc)(rtcUnhandledStunRequest request);


On the contrary, the C callback should have a user pointer as there is no way to pass context otherwise. It should be stored by capturing it in the C++ callback.

Hmm, I think this def might be left over from a previous attempt. I've removed it.

paullouisageneau · 2025-01-17T12:39:17Z

src/global.cpp

+
+	PLOG_DEBUG << "Adding listener for unhandled STUN requests";
+
+	UnhandledStunRequestHandler *handler = (UnhandledStunRequestHandler*)calloc(1, sizeof(UnhandledStunRequestHandler));


Please don't use C-style functions in C++. Here, you should use a unique_ptr or directly set the object as map value. However, this should not be necessary anymore if you remove the user pointer.

paullouisageneau · 2025-01-17T12:42:29Z

src/global.cpp

+	handler->callback = std::move(callback);
+	handler->userPtr = userPtr;
+
+	unboundStunCallbacks[port] = handler;


There should be a mutex protecting the map otherwise the function is not thread-safe.

I've removed the map.

paullouisageneau · 2025-01-17T12:53:52Z

src/global.cpp

@@ -88,6 +93,61 @@ std::shared_future<void> Cleanup() { return impl::Init::Instance().cleanup(); }

 void SetSctpSettings(SctpSettings s) { impl::Init::Instance().setSctpSettings(std::move(s)); }

+std::map<int, UnhandledStunRequestHandler *> unboundStunCallbacks;


Should callbacks be specified by port or by bind address? Just asking because it needs to be consistent with libjuice. I think port makes sense, since the user is typically going to set the same bindAddress everytime, and the any address is quite tedious to handle.

The libjuice PR uses ${host}:${port} as the key.

Since the conn_mux.c uses udp_socket_config_t to bind a UDP port, I wanted to make sure the user could explicitly bind both IPv4 and IPv6 addresses with:

juice_mux_listen("0.0.0.0", port, &cb, NULL); juice_mux_listen("::", port, &cb, NULL);

~~I could be misreading it but I think these lines mean you need to pass one address family or the other. For example passing NULL as the bind_address will not cause it to bind to both.~~

Actually I'm not sure that's correct, passing NULL seems to bind all IPv4 and IPv6 interfaces.

achingbrain mentioned this pull request Jun 14, 2024

feat: WebRTC-Direct support for Node.js libp2p/js-libp2p#2583

Draft

12 tasks

achingbrain mentioned this pull request Nov 18, 2024

feat: support setting ICE ufrag and pwd, disabling fingerprint validation and specifying certificates murat-dogan/node-datachannel#256

Draft

achingbrain force-pushed the feat/expose-callback-for-unknown-stun-ufrag branch from 4de4a6d to af8f6c8 Compare January 14, 2025 14:43

achingbrain force-pushed the feat/expose-callback-for-unknown-stun-ufrag branch from 41e2380 to 311fea0 Compare January 14, 2025 16:54

fix: support multiple callbacks for different ports

9157c0c

paullouisageneau reviewed Jan 17, 2025

View reviewed changes

chore: pr comments

4e0a5d8

achingbrain force-pushed the feat/expose-callback-for-unknown-stun-ufrag branch from 2dca4c7 to 4e0a5d8 Compare January 17, 2025 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add callback for unhandled STUN requests #1211

feat: add callback for unhandled STUN requests #1211

achingbrain commented Jun 14, 2024 •

edited

Loading

achingbrain commented Jun 14, 2024 •

edited

Loading

xicilion commented Jun 14, 2024

xicilion commented Jun 14, 2024

achingbrain commented Jan 16, 2025 •

edited

Loading

paullouisageneau Jan 17, 2025

achingbrain Jan 17, 2025

paullouisageneau Jan 17, 2025

achingbrain Jan 17, 2025

paullouisageneau Jan 17, 2025

achingbrain Jan 17, 2025

paullouisageneau Jan 17, 2025

achingbrain Jan 17, 2025

paullouisageneau Jan 17, 2025

achingbrain Jan 17, 2025

paullouisageneau Jan 17, 2025

achingbrain Jan 17, 2025

paullouisageneau Jan 17, 2025

achingbrain Jan 17, 2025

paullouisageneau Jan 17, 2025

achingbrain Jan 17, 2025 •

edited

Loading


		#endif

		void OnUnhandledStunRequest ([[maybe_unused]] std::string host, [[maybe_unused]] int port, [[maybe_unused]] UnhandledStunRequestCallback callback, [[maybe_unused]] void *userPtr) {


		PLOG_DEBUG << "Adding listener for unhandled STUN requests";

		UnhandledStunRequestHandler handler = (UnhandledStunRequestHandler)calloc(1, sizeof(UnhandledStunRequestHandler));

		@@ -88,6 +93,61 @@ std::shared_future<void> Cleanup() { return impl::Init::Instance().cleanup(); }

		void SetSctpSettings(SctpSettings s) { impl::Init::Instance().setSctpSettings(std::move(s)); }

		std::map<int, UnhandledStunRequestHandler *> unboundStunCallbacks;

feat: add callback for unhandled STUN requests #1211

Are you sure you want to change the base?

feat: add callback for unhandled STUN requests #1211

Conversation

achingbrain commented Jun 14, 2024 • edited Loading

achingbrain commented Jun 14, 2024 • edited Loading

xicilion commented Jun 14, 2024

xicilion commented Jun 14, 2024

achingbrain commented Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achingbrain Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

achingbrain commented Jun 14, 2024 •

edited

Loading

achingbrain commented Jun 14, 2024 •

edited

Loading

achingbrain commented Jan 16, 2025 •

edited

Loading

achingbrain Jan 17, 2025 •

edited

Loading