Skip to content
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.

Sign up for GitHub

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

feat: add callback for unhandled STUN requests #1211

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+190 −2
Open

feat: add callback for unhandled STUN requests #1211

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
311fea0
feat: add callback for unhandled STUN requests
achingbrain Jun 14, 2024
9157c0c
fix: support multiple callbacks for different ports
achingbrain Jan 16, 2025
4e0a5d8
chore: pr comments
achingbrain Jan 17, 2025
fcfcc4f
Refactor API into an IceUdpMuxListener object
paullouisageneau Jan 23, 2025
b8e5348
Fix warning with libnice
paullouisageneau Jan 23, 2025
d15c2cd
chore: whitespace
achingbrain Feb 6, 2025
b47bae7
Merge branch 'paullouisageneau:master' into feat/expose-callback-for-…
achingbrain Feb 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion deps/libjuice
Open in desktop
Submodule libjuice updated 9 files
+13 −0 include/juice/juice.h
+8 −4 src/agent.c
+67 −1 src/conn.c
+2 −0 src/conn.h
+34 −8 src/conn_mux.c
+1 −0 src/conn_mux.h
+1 −0 src/conn_poll.c
+1 −0 src/conn_thread.c
+9 −11 src/juice.c
16 changes: 16 additions & 0 deletions include/rtc/global.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,22 @@ RTC_CPP_EXPORT void InitLogger(LogLevel level, LogCallback callback = nullptr);
RTC_CPP_EXPORT void Preload();
RTC_CPP_EXPORT std::shared_future<void> Cleanup();

struct UnhandledStunRequest {
std::string localUfrag;
std::string remoteUfrag;
std::string remoteHost;
uint16_t remotePort;
};

RTC_CPP_EXPORT typedef std::function<void(UnhandledStunRequest request, void* userPtr)> UnhandledStunRequestCallback;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right that it's not needed if you store only a pointer to an external function object, since the concern is pushed to the user. However, I don't think it's a good idea, as stated in #1211 (comment).


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

RTC_CPP_EXPORT void OnUnhandledStunRequest(std::string host, int port, UnhandledStunRequestCallback callback = nullptr, void *userPtr = nullptr);

struct SctpSettings {
// For the following settings, not set means optimized default
optional<size_t> recvBufferSize; // in bytes
Expand Down
14 changes: 14 additions & 0 deletions include/rtc/rtc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,20 @@ typedef void(RTC_API *rtcAvailableCallbackFunc)(int id, void *ptr);
typedef void(RTC_API *rtcPliHandlerCallbackFunc)(int tr, void *ptr);
typedef void(RTC_API *rtcRembHandlerCallbackFunc)(int tr, unsigned int bitrate, void *ptr);

// Handle STUN requests with unexpected ufrags

typedef struct {
const char * ufrag;
const char * pwd;
uint8_t family;
const char * address;
uint16_t port;
} rtcUnhandledStunRequest;

typedef void(RTC_API *rtcUnhandledStunRequestCallbackFunc)(rtcUnhandledStunRequest request);

RTC_C_EXPORT void rtcOnUnhandledStunRequest(const char *host, int port, rtcUnhandledStunRequestCallbackFunc callback);

// Log

// NULL cb on the first call will log to stdout
Expand Down
60 changes: 60 additions & 0 deletions src/global.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@
#include "impl/init.hpp"

#include <mutex>
#include <map>

#if !USE_NICE
#include <juice/juice.h>
#endif

namespace {

Expand Down Expand Up @@ -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;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@achingbrain achingbrain Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Correct, NULL binds on IPv6 and IPv4. That's what you want in most cases.


#if !USE_NICE

void InvokeUnhandledStunRequestCallback (const juice_mux_binding_request *info, void *user_ptr) {
PLOG_DEBUG << "Invoking unhandled STUN request callback";

UnhandledStunRequestHandler *handler = (struct UnhandledStunRequestHandler*)user_ptr;

if (handler->callback) {
handler->callback({
std::string(info->local_ufrag),
std::string(info->remote_ufrag),
std::string(info->address),
info->port
}, handler->userPtr);
} else {
PLOG_DEBUG << "No unhandled STUN request callback configured for port " << info->port;
}
}

#endif

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

host should also be optional to allow any address (it would pass NULL to libjuice)

I guess the only problem with this is the invocation to unbind a listener with a host now looks a bit awkward, certainly in js:

listenIceUdpMux(port, null, host)

How about passing a bind_address struct with host/port fields as the first argument instead? That way the host field can be omitted if it should bind to all available addresses.

#if USE_NICE
PLOG_WARNING << "BindStunListener is not supported with libnice, please use libjuice";
#else
if (callback == NULL) {
PLOG_DEBUG << "Removing unhandled STUN request listener";

free(unboundStunCallbacks.at(port));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, making the memory management unsafe and delegating it to the user is a complete footgun, and even if the user manages lifetimes correctly, simply changing the function on user side would create a race condition.

More importantly, it prevents from using lambdas, which removes at lot of the appeal of callbacks:

rtc::ListenIceUdpMux(port, [](IceUdpMuxRequest request) {
 // Do something
});

unboundStunCallbacks.erase(port);

// call with NULL callback to unbind
if (juice_mux_listen(host.c_str(), port, NULL, NULL) < 0) {
throw std::runtime_error("Could not unbind STUN listener");
}

return;
}

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

UnhandledStunRequestHandler *handler = (UnhandledStunRequestHandler*)calloc(1, sizeof(UnhandledStunRequestHandler));
handler->callback = std::move(callback);
handler->userPtr = userPtr;

unboundStunCallbacks[port] = handler;

if (juice_mux_listen(host.c_str(), port, &InvokeUnhandledStunRequestCallback, handler) < 0) {
throw std::invalid_argument("Could not add listener for unhandled STUN requests");
}
#endif
}

RTC_CPP_EXPORT std::ostream &operator<<(std::ostream &out, LogLevel level) {
switch (level) {
case LogLevel::Fatal:
Expand Down
Loading