7 ms·

I am very curious to understand better how this stacks up against the usual C++ WebRTC implementation when code size isn't an issue, particularly with respect t

by saurik 7y ago

I am very curious to understand better how this stacks up against the usual C++ WebRTC implementation when code size isn't an issue, particularly with respect to flexibility. (I work on a project that does VPN services over WebRTC, am doing stuff like WebRTC tunneled over WebRTC, have WebRTC routing over LwIP, and we are working on improving the congestion control algorithms.) My stripped down compile of that library--which admittedly is dropping most of the audio/video functionality as I only care about DataChannels, so maybe if I were doing video I'd be freaking out at the size of the stack (though I would imagine the video codecs would dominate the size, no?)--is clocking in at about 7MB, which is much larger than 200kB but nowhere near the point where I start to care: I mean, the default buffer size per channel in the library is a crazy large 16MB buffer, so I find myself not caring much about the size of my text segment ;P. (Are embedded systems really that constrained these days?) One thing I have enjoyed about the C++ build is that it has tons and tons of knobs for me to mess with in the binding API... I have to reimplement the network detection and am using two separate socket implementations (which is all pretty trivial as just about everything can be subclassed and replaced), I mess with the offers and answers before handing them back/forth through the stack (which is easy as there is an object-oriented model for all of the data structures; I mean, it isn't a great model, but it is at least a model), I am able to easily assign work to occur on specific threads (and have been able to share a signaling thread while splitting networking threads across backends), and I at times reach into the heart of the system (without any code modifications, though with a really hilarious template trick to help me access private C++ class members) to pull up the SCTP stack internals. (Note that with only one arguable-exception we do everything we do currently without any modifications to WebRTC's code.) Is this implementation going to be better for me, in maybe having fewer layers of abstraction between me and the underlying state machines, or worse, due to trying to target "embedded systems" in a programming language that tends to be less flexible (and so between those two considerations maybe leading me to have less ability to customize it without having to rewrite or hack on it directly)? I ask because I know a lot of other people see C and think "ah, finally this is simple", but I have not had any issues with the C++ codebase (the secret is to not even try to use Google's build system: just take all the .cc files you need--and only the ones you need--and link them together). (Oh, maybe even really simple: I bet this isn't going to support Windows?)

Sean-Der 7y ago
I know Orchid[0]! Me and another Pion developer were just talking about, we both are really impressed. WebRTC is such an amazing technology that it can enable stuff like this (and it was never planned). We were trying to scheme a way to convince you to use Pion :p > I am very curious to understand better how this stacks up against the usual C++ WebRTC implementation when code size isn't an issue, I am biased, but I think if you only care about DataChannels they should be pretty equivalent! Both libraries use libusrsctp/OpenSSL for all the heavy lifting, so that should be the same. They do have different ICE implementations though, so that is probably going to be the first place you will hit issues. I encourage you to try it out though. If you hit any bugs/have any questions I would love to help! > as just about everything can be subclassed and replaced Oh interesting. I think you are right, this library wasn't really designed to be inherited/mutated. We do provide a struct with non-standard settings you can flip, so maybe we can add the things you need in there? > I bet this isn't going to support Windows? I plan on supporting it! I don't have access to a Windows machine, but I hope I can just put it in travis and never touch it again :) If you want to reach out via email/GitHub issues I am happy to discuss things further. That is a pretty question heavy comment, so I think I am missing some stuff on mobile. [0] https://github.com/OrchidTechnologies/orchid https://github.com/OrchidTechnologies/orchid
- saurik 7y ago
  I spent about five minutes just now--on my iPhone, to kill time before falling asleep, so I notably didn't even have a keyboard--barely glancing at your code on GitHub, and found a buffer overflow :( in your data channel DCEP notification implementation (and then spent like a half hour verifying it on my iPhone and typing this comment on my iPhone, so I am sadly now much more awake than when I started ;P). You have MAX_DATA_CHANEL_NAME_LEN (which is misspelled; I was very confused when the GitHub search couldn't find it ;P) set to 255 and in RtcDataChannel you allocate name with that size (already suspicious, as in most other places you + 1 for a NUL byte; this is what caused me to search for usages: in samples/Common.c you incorrectly assume it is terminated, btw); but, in handleDcepPacket you get a 16-bit value for the length which you pass through dataChannelOpenFunc to onSctpSessionDataChannelOpen... and then it is that length (as pNameLen) which gets passed to STRNCPY (which I verified is a #define for strncpy) instead of the size of the buffer. I had momentarily considered "maybe this field fundamentally can't be larger than 255 for some other reason I don't see here" (as I honestly couldn't remember what the length limitations for these things are), but the standard even makes clear that these fields can go up to the full 16-bit length: > The DATA_CHANNEL_OPEN messages contains two variable length fields: the protocol and the label. A receiver must be prepared to receive DATA_CHANNEL_OPEN messages where these field have the maximum length of 65535 bytes. (This then made me do a quick search for STRNCPY in your code, and I am noticing you are using the wrong length constants for most of the calls in the SDP parser; this happens to work, because they are all defined to be the same number--255--but is still highly suspicious and would make me want to do a much more thorough audit. Really, my recommendation would be to never call STRNCPY directly: abstract it behind a macro that enforces you pass a direct array and an offset so it can do sizeof() to get the right size at compile time.) > We were trying to scheme a way to convince you to use Pion :p Honestly, I guess I am just of the firm belief that this is 2020, and no one should be writing code in "Pure C" under any circumstance :/. You might totally find a place where I made a mistake like this in my code (as I am, after all, still using C++, and I have very few fundamental guarantees), but when I fix it it will fix that entire class of bug for all of my code everywhere, as I have templated abstractions for things like buffer management (where I have even been trying to slip in some pseudo-dependent types to elide runtime checks) and am able to use deconstructors to manage my resources; my biggest issue is dealing with asynchronous coroutines running code in objects as they get deallocated, but even there I have been able to design a mostly type-safe abstraction (that if nothing else should catch the coding mistake at runtime and safely terminate the process). Can I maybe convince you to rewrite this in C++20 or Rust? :(

by saurik 7y ago

Sean-Der 7y ago

I know Orchid[0]! Me and another Pion developer were just talking about, we both are really impressed. WebRTC is such an amazing technology that it can enable stuff like this (and it was never planned). We were trying to scheme a way to convince you to use Pion :p > I am very curious to understand better how this stacks up against the usual C++ WebRTC implementation when code size isn't an issue, I am biased, but I think if you only care about DataChannels they should be pretty equivalent! Both libraries use libusrsctp/OpenSSL for all the heavy lifting, so that should be the same. They do have different ICE implementations though, so that is probably going to be the first place you will hit issues. I encourage you to try it out though. If you hit any bugs/have any questions I would love to help! > as just about everything can be subclassed and replaced Oh interesting. I think you are right, this library wasn't really designed to be inherited/mutated. We do provide a struct with non-standard settings you can flip, so maybe we can add the things you need in there? > I bet this isn't going to support Windows? I plan on supporting it! I don't have access to a Windows machine, but I hope I can just put it in travis and never touch it again :) If you want to reach out via email/GitHub issues I am happy to discuss things further. That is a pretty question heavy comment, so I think I am missing some stuff on mobile. [0] https://github.com/OrchidTechnologies/orchid https://github.com/OrchidTechnologies/orchid

saurik 7y ago
I spent about five minutes just now--on my iPhone, to kill time before falling asleep, so I notably didn't even have a keyboard--barely glancing at your code on GitHub, and found a buffer overflow :( in your data channel DCEP notification implementation (and then spent like a half hour verifying it on my iPhone and typing this comment on my iPhone, so I am sadly now much more awake than when I started ;P). You have MAX_DATA_CHANEL_NAME_LEN (which is misspelled; I was very confused when the GitHub search couldn't find it ;P) set to 255 and in RtcDataChannel you allocate name with that size (already suspicious, as in most other places you + 1 for a NUL byte; this is what caused me to search for usages: in samples/Common.c you incorrectly assume it is terminated, btw); but, in handleDcepPacket you get a 16-bit value for the length which you pass through dataChannelOpenFunc to onSctpSessionDataChannelOpen... and then it is that length (as pNameLen) which gets passed to STRNCPY (which I verified is a #define for strncpy) instead of the size of the buffer. I had momentarily considered "maybe this field fundamentally can't be larger than 255 for some other reason I don't see here" (as I honestly couldn't remember what the length limitations for these things are), but the standard even makes clear that these fields can go up to the full 16-bit length: > The DATA_CHANNEL_OPEN messages contains two variable length fields: the protocol and the label. A receiver must be prepared to receive DATA_CHANNEL_OPEN messages where these field have the maximum length of 65535 bytes. (This then made me do a quick search for STRNCPY in your code, and I am noticing you are using the wrong length constants for most of the calls in the SDP parser; this happens to work, because they are all defined to be the same number--255--but is still highly suspicious and would make me want to do a much more thorough audit. Really, my recommendation would be to never call STRNCPY directly: abstract it behind a macro that enforces you pass a direct array and an offset so it can do sizeof() to get the right size at compile time.) > We were trying to scheme a way to convince you to use Pion :p Honestly, I guess I am just of the firm belief that this is 2020, and no one should be writing code in "Pure C" under any circumstance :/. You might totally find a place where I made a mistake like this in my code (as I am, after all, still using C++, and I have very few fundamental guarantees), but when I fix it it will fix that entire class of bug for all of my code everywhere, as I have templated abstractions for things like buffer management (where I have even been trying to slip in some pseudo-dependent types to elide runtime checks) and am able to use deconstructors to manage my resources; my biggest issue is dealing with asynchronous coroutines running code in objects as they get deallocated, but even there I have been able to design a mostly type-safe abstraction (that if nothing else should catch the coding mistake at runtime and safely terminate the process). Can I maybe convince you to rewrite this in C++20 or Rust? :(