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

rtp forward with ipv6 udp socket does not work on macos #2051

Closed
kazzmir opened this issue Apr 6, 2020 · 6 comments
Closed

rtp forward with ipv6 udp socket does not work on macos #2051

kazzmir opened this issue Apr 6, 2020 · 6 comments

Comments

@kazzmir
Copy link
Contributor

kazzmir commented Apr 6, 2020

When setting up an RTP forward in the videoroom plugin, janus will create an ipv6 udp socket but then select an ipv4 or ipv6 sockaddr address depending on how the rtp forward was set up. Unfortunately calling sendto() on an ipv6 socket to an ipv4 address does not work on macos. I realize macos is not a first tier citizen in terms of janus support, but it would be a fairly minor change to support macos here.

The setup is that I have a gst-launch-1.0 process listening with a udpsrc element on 0.0.0.0:4005, and I create an RTP forward in janus with host="127.0.0.1" and port=4005.

The error shows up in the janus log

Error forwarding RTP audio packet for me... Invalid argument (len=98)...

The following C program also demonstrates the issue

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <netdb.h>
#include <errno.h>
#include <string.h>
#include <arpa/inet.h>

int main(){
    int ok = 0;
    int port = 5454;
    int udp = socket(AF_INET6, SOCK_DGRAM, IPPROTO_UDP);
    // int udp = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
    if (udp <= 0){
        printf("Unable to create socket: %s\n", strerror(errno));
        return 1;
    }

    printf("Created udp socket %d\n", udp);

    /*
    int v6only = 0;
    int ok = setsockopt(udp, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, sizeof(v6only));
    if (ok < 0){
        printf("Could not set IPV6_V6ONLY=%d: %s\n", v6only, strerror(errno));
        return 1;
    }
    */

    /*
    struct sockaddr_in address;
    address.sin_family = AF_INET;
    inet_pton(AF_INET, "127.0.0.1", &address.sin_addr);
    address.sin_port = htons(port);
    */
    struct sockaddr_in6 address;
    address.sin6_family = AF_INET6;
    inet_pton(AF_INET6, "::1", &address.sin6_addr);
    address.sin6_port = htons(port);

    char buffer[10];
    sprintf(buffer, "test");
    ok = sendto(udp, buffer, strlen(buffer), 0, (struct sockaddr *) &address, sizeof(address));
    if (ok < 0){
        printf("Unable to send data: %s\n", strerror(errno));
        return 1;
    } else {
        printf("Sent %d bytes\n", ok);
    }

    close(udp);
}

Test this with

$ nc -u -l 5454
# In another terminal
$ clang x.c -o x
$ ./x

The word "test" should show up in the nc output.

On linux the socket can be ipv6 and the address can be ipv4, but on macos the socket and address must both be ipv6 or both be ipv4, otherwise sendto() will return -EINVAL.

The behavior in janus was changed in #1778

I was able to resolve this issue by making the following small change to plugins/janus_videoplugin.c

                if(publisher->udp_sock <= 0) {
                        // publisher->udp_sock = socket(AF_INET6, SOCK_DGRAM, IPPROTO_UDP);
                        publisher->udp_sock = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
                        int v6only = 0;
                        if(publisher->udp_sock <= 0
                                        // || setsockopt(publisher->udp_sock, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, sizeof(v6only)) != 0
                             ) {

A more general simple fix is to use the check used elsewhere, which is if serv_addr.sin_family == AF_INET{ .. do ipv4 stuff .. } else { .. do ipv6 suff .. }. Would that be a reasonable compromise rather than having some #ifdef MACOS ? I can make a PR for this if so.

BTW I also tried having gst-launch-1.0 listen on an ipv6 address rather than ipv4, but gst just dies as soon as it receives the first udp packet from janus.

$ gst-launch-1.0 udpsrc uri="udp://[::0]:4000"  ...
Setting pipeline to PAUSED ...
Pipeline is live and does not need PREROLL ...
Setting pipeline to PLAYING ...
New clock: GstSystemClock
ERROR: from element /GstPipeline:pipeline0/GstUDPSrc:udpsrc0: Internal data stream error.
Additional debug info:
gstbasesrc.c(3072): void gst_base_src_loop(GstPad *) (): /GstPipeline:pipeline0/GstUDPSrc:udpsrc0:
streaming stopped, reason not-linked (-1)
Execution ended after 0:00:04.675785000
Setting pipeline to PAUSED ...
Setting pipeline to READY ...
Setting pipeline to NULL ...
Freeing pipeline ...
@lminiero
Copy link
Member

lminiero commented Apr 6, 2020

Your suggestion will always create an IPv4 socket. Won't that fail if we need to send to IPv6? A better solution probably is to anticipate the host resolution part, so that when it comes to creating the socket we know what we're sending to and we know what we should create. A pull request would be welcome, thanks!

@kazzmir
Copy link
Contributor Author

kazzmir commented Apr 6, 2020

Yes you are right, the change I made for now is a local hack to get my system to work. The proper fix is to check the server address and create the socket based on that. I will submit a PR soon.

@kazzmir
Copy link
Contributor Author

kazzmir commented Apr 6, 2020

#2053 created this PR.

I didn't realize that there is a single udp socket and multiple addresses to send to, so the PR is slightly more complicated to handle this situation.

@lminiero
Copy link
Member

lminiero commented Apr 8, 2020

Looking at your test code, it looks like we won't be able to just add a check on whether IPV6_V6ONLY is defined to do different things. We can't check whether setsockopt fails either, because it is apparently succeeding for you, or you'd get an error creating the forwarder (unless this is what you were getting and you just compiled it out?). The fact it only fails when it gets to send/sendto means we'll have to use preprocessors just for the mac use case.

@jsm222
Copy link
Contributor

jsm222 commented Jun 15, 2020

I made a patch for FreeBSD trying to solve the same problem:
It reads ws.ip from config and determines its type also see
warmcat/libwebsockets#1947

-- transports/janus_websockets.c.orig  2020-06-01 08:39:34 UTC
+++ transports/janus_websockets.c
@@ -274,7 +274,7 @@ static const char *janus_websockets_reason_string(enum
 #if (LWS_LIBRARY_VERSION_MAJOR >= 4)
 static lws_retry_bo_t pingpong = { 0 };
 #endif
-
+struct in_addr addr;
 /* Helper method to return the interface associated with a local IP address */
 static char *janus_websockets_get_interface_name(const char *ip) {
        struct ifaddrs *addrs = NULL, *iap = NULL;
@@ -553,13 +553,6 @@ int janus_websockets_init(janus_transport_callbacks *c
                /* Force single-thread server */
                wscinfo.count_threads = 1;
 
-               /* Create the base context */
-               wsc = lws_create_context(&wscinfo);
-               if(wsc == NULL) {
-                       JANUS_LOG(LOG_ERR, "Error creating libwebsockets context...\n");
-                       janus_config_destroy(config);
-                       return -1;      /* No point in keeping the plugin loaded */
-               }
 
                /* Setup the Janus API WebSockets server(s) */
                item = janus_config_get(config, config_general, janus_config_type_item, "ws");
@@ -580,12 +573,22 @@ int janus_websockets_init(janus_transport_callbacks *c
                        item = janus_config_get(config, config_general, janus_config_type_item, "ws_ip");
                        if(item && item->value) {
                                ip = (char *)item->value;
+                               if(inet_net_pton(AF_INET, ip, &addr, sizeof(addr))>0) {
+                               wscinfo.options |= LWS_SERVER_OPTION_DISABLE_IPV6;
+                               }
                                char *iface = janus_websockets_get_interface_name(ip);
                                if(iface == NULL) {
                                        JANUS_LOG(LOG_WARN, "No interface associated with %s? Falling back to no interface...\n", ip);
                                }
                                ip = iface;
                        }
+               /* Create the base context */
+               wsc = lws_create_context(&wscinfo);
+               if(wsc == NULL) {
+                       JANUS_LOG(LOG_ERR, "Error creating libwebsockets context...\n");
+                       janus_config_destroy(config);
+                       return -1;      /* No point in keeping the plugin loaded */
+               }
                        /* Prepare context */
                        struct lws_context_creation_info info;
                        memset(&info, 0, sizeof info);

@lminiero
Copy link
Member

lminiero commented Jul 1, 2022

Closing as we've already added some IPv6 checks.

@lminiero lminiero closed this as completed Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants