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.

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 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
16 changes: 16 additions & 0 deletions include/rtc/global.hpp
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?


struct UnhandledStunRequestHandler {
UnhandledStunRequestCallback callback;
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.

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

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!


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
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);
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


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
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

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.


#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!

#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.

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));
Copy link
Owner

Choose a reason for hiding this comment

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

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.

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!

handler->callback = std::move(callback);
handler->userPtr = userPtr;

unboundStunCallbacks[port] = handler;
Copy link
Owner

Choose a reason for hiding this comment

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

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

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.


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