Do no undefine __APPLE__ on mac/ios builds

[pps83rbx]

This was already reported and simply closed before. This is a bug that has to be fixed.

__APPLE__ is a known identifier for apple builds (mac/ios), this is insane to undefine it. More over, this isn't supported, as identifiers with leading underscores are reserved. You aren't supposed to define/undefine these, I hope it's not a surprise for you. If you need something special, then it should have been SCTP_APPLE or whatever.
The way it is now is kind of crazy: some structs have different layout depending of presence of __APPLE__ define. I hope you do not expect that any unrelated code that happens to use usrsctp library will also need to undefine __APPLE__ in their builds!? I also hope that tests for __APPLE__ in public headers doesn't affect layouts of these strucs in way that would corrupt anything that tries to use usrsctp on mac/ios. I'd be scared to touch this library with a long pole in a commercial product if I didn't do full code review for each use of __APPLE__. Please, fix the bug to avoid these issues.

[lgrahl]

@tuexen already said he agrees with you and that they are very much willing to accept a pull request.

[pps83rbx]

I just did quick code review and I'm totally shocked that this code doesn't bomb. It's used by chrome in webrtc, strange that they didn't notice it.
The state of things is disaster and national emergency, as this isn't only about __APPLE__ but about any other platform. Abuse of ifdefs in headers based on any of these defines makes it unusable as-is in a project where you cannot be certain that all defines will be the same across all compilation units (which is never the case).

The problem is that I don't use/work with/on usrsctp. It's used/pulled in by another third party library, which in turn also pulled in by another third party library. I just noticed weird stuff going on and started investigation and ... I'm not sure if I can use/trust all of these libraries now. Complete disaster.

[lgrahl]

I also did a quick review... this is dead code unless someone defines SCTP_USE_NSS_SHA1. So, you can calm down now. 🙂

[lgrahl]

And apparently none of the browsers define it, so they aren't aware (or were aware and decided not to report it):

• Chrome: https://cs.chromium.org/chromium/src/third_party/usrsctp/BUILD.gn?l=96&rcl=38de9b980a6fd8ca452c39b2db1dec5639459976
• Firefox: https://dxr.mozilla.org/mozilla-central/search?q=SCTP_USE_NSS_SHA1&redirect=false
• WebKit: https://github.com/WebKit/webkit/blob/9d678322281ccd07909759af90ee010e0c34b1c7/Source/ThirdParty/libwebrtc/CMakeLists.txt#L1499

[pps83rbx]

I also did a quick review... this is dead code unless someone defines SCTP_USE_NSS_SHA1. So, you can calm down now. 🙂

I wasn't at all concerned by this piece. It's "safe" even though ugly hasck as the code says.

These are the problems I'm concerned about:

sctp_pcb.h:
https://github.com/sctplab/usrsctp/blob/master/usrsctplib/netinet/sctp_pcb.h#L193
https://github.com/sctplab/usrsctp/blob/master/usrsctplib/netinet/sctp_pcb.h#L227
https://github.com/sctplab/usrsctp/blob/master/usrsctplib/netinet/sctp_pcb.h#L308
https://github.com/sctplab/usrsctp/blob/master/usrsctplib/netinet/sctp_pcb.h#L455
https://github.com/sctplab/usrsctp/blob/master/usrsctplib/netinet/sctp_pcb.h#L541
https://github.com/sctplab/usrsctp/blob/master/usrsctplib/netinet/sctp_pcb.h#L558
https://github.com/sctplab/usrsctp/blob/master/usrsctplib/netinet/sctp_pcb.h#L645
https://github.com/sctplab/usrsctp/blob/master/usrsctplib/netinet/sctp_pcb.h#L654

sctp_structs.h:
https://github.com/sctplab/usrsctp/blob/master/usrsctplib/netinet/sctp_structs.h#L190
https://github.com/sctplab/usrsctp/blob/master/usrsctplib/netinet/sctp_structs.h#L252
https://github.com/sctplab/usrsctp/blob/master/usrsctplib/netinet/sctp_sysctl.h#L131

usrsctp.h:
https://github.com/sctplab/usrsctp/blob/master/usrsctplib/usrsctp.h#L122

Are any of these headers exported (used outside of usrsctp)?

And apparently none of the browsers define it, so they aren't aware (or were aware and decided not to report it):

• Chrome: https://cs.chromium.org/chromium/src/third_party/usrsctp/BUILD.gn?l=96&rcl=38de9b980a6fd8ca452c39b2db1dec5639459976
• Firefox: https://dxr.mozilla.org/mozilla-central/search?q=SCTP_USE_NSS_SHA1&redirect=false
• WebKit: https://github.com/WebKit/webkit/blob/9d678322281ccd07909759af90ee010e0c34b1c7/Source/ThirdParty/libwebrtc/CMakeLists.txt#L1499

This is not correct. See here: WebKit/WebKit@7662de8 ( Search for -U__APPLE__ there. That's how I ended up in usrsctp by the way). Browsers imo have to use usrsctp as userland build, that is usrsctp would undefine most of standard defines from build chains. But then later when usrsctp headers are included obviously it will not be the case. The other issue: seems like usrsctp expects all these endless defines to be present when using it (all those __Userland stuff).

[lgrahl]

Right, so IIRC the problem you're describing is not the fact that __APPLE__ may be undefined but rather that __APPLE__ must be undefined when building usrsctp to get it to work in userspace on iOS/Mac. This results in the CFLAGS propagating to your application which messes up the build for iOS/Mac. Is that correct?

Are any of these headers exported (used outside of usrsctp)?

usrsctp.h is but the rest is not. So, if I interpret the code correctly and you're including a built version of usrsctp where the defines (and undefines) have not been propagated to your library, then including usrsctp.h will redefine struct sockaddr_conn with a different alignment.

The other issue: seems like usrsctp expects all these endless defines to be present when using it (all those __Userland stuff).

This has been resolved apart from a single define. See the discussion in #275.

[pps83rbx]

Right, so IIRC the problem you're describing is not the fact that __APPLE__ may be undefined but rather that __APPLE__ must be undefined when building usrsctp to get it to work in userspace on iOS/Mac. This results in the CFLAGS propagating to your application which messes up the build for iOS/Mac. Is that correct?

usrsctp treats __APPLE__ as a define that says that it's a kernel-level (non-userland) build of usrsctp, same goes for other similar platform defines. So, usrsctp is built with some defines/undefines, but when it's used from external code it's not very reasonable to require that users of the library also define the same defines that were used to build it (if usrsctp needs that, it should have some config.h that would have the same defines), and it's absolutely ridiculous to expect that user code would undef platform defines. That's just unacceptable.

Are any of these headers exported (used outside of usrsctp)?

usrsctp.h is but the rest is not. So, if I interpret the code correctly and you're including a built version of usrsctp where the defines (and undefines) have not been propagated to your library, then including usrsctp.h will redefine struct sockaddr_conn with a different alignment.

usrscpt.h does different layout for struct sockaddr_conn. If user code doesn't directly try to read/write sconn_family then it should be ok. Still, it's so brittle the way it is with so many ifdefs in headers taht affect struct layout. It's irrelevant if they are correct, it's very difficult for anybody unfamiliar with the code to coclude if it's not broken/corrupted.

The other issue: seems like usrsctp expects all these endless defines to be present when using it (all those __Userland stuff).

This has been resolved apart from a single define. See the discussion in #275.

[pps83rbx]

It would be better if usrsctp had consistent defines (USRSCTP_USERLAND, USRSCTP_SOMETHING_ELSE) and preferably wouldn't require user code to also define the same defines when using usrscpt headers. That usually requires some kind of config.h to be exported as a public header.
I use webrtc and they end up having to define all kinds of defines from all kinds of libs that insist that user code does so. It's a wild zoo of defines and it's difficult to even reason why and where from some of these come. __Userland_on_Darwin particularly stood out as most unusual when I saw it.

[lgrahl]

Already pointed out that you don't need to define anything when including usrsctp.h in your application. The (private) defines are only needed when building and they don't matter afterwards... with the exception of SCTP_DEBUG as mentioned in #275 and __APPLE__ for iOS/Mac which is clear by now.

Yep, they aren't prefixed and the code certainly isn't going to win a beauty contest... but that shouldn't be our priority here. Unless you want to go for it. 🙂

[tuexen]

This issue should be fixed by: 5be0c25.