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

Fix wrong size_t* to int* conversion in 64bit Big-Endian hosts #637

Merged

Conversation

ibc
Copy link
Member

@ibc ibc commented Aug 13, 2021

- Fixes #636
- Ensure that we have RTP or RTCP packet length into a `int` variable since `srtp_(un)protect_xxxx()` methods expect a pointer to `int` and modify the length referenced by the given pointer.
Copy link
Collaborator

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

This change makes sense to me

@ibc
Copy link
Member Author

ibc commented Aug 13, 2021

This change makes sense to me

Thanks. Will wait a few days for @xia-chu.

@nazar-pc
Copy link
Collaborator

I'm sure it is possible to run a binary for any arch (and probably endianness) on Linux with binfmt support.
I was compiling ARM and AARCH64 apps this way and even tricked Docker Hub to build ARM images on their infra, so maybe big-endian version of PPC would work too, though I'm too lazy to test it myself.

@nazar-pc
Copy link
Collaborator

nazar-pc commented Aug 13, 2021

I ❤️ Linux: decided to give it a spin on s390x (notable BE platform) Ubuntu container under Docker.

Both before and after this patch following worker tests were failing identically:

-------------------------------------------------------------------------------
Scenario: RTCP Feedback PS RPSI parsing
  parse FeedbackPsRpsiPacket
-------------------------------------------------------------------------------
../test/src/RTC/RTCP/TestFeedbackPsRpsi.cpp:50
...............................................................................

../test/src/RTC/RTCP/TestFeedbackPsRpsi.cpp:40: FAILED:
  REQUIRE( item->GetPayloadType() == payloadType )
with expansion:
  2 == 1

-------------------------------------------------------------------------------
Scenario: RTCP Feedback PS VBCM parsing
  parse FeedbackPsVbcmPacket
-------------------------------------------------------------------------------
../test/src/RTC/RTCP/TestFeedbackPsVbcm.cpp:56
...............................................................................

../test/src/RTC/RTCP/TestFeedbackPsVbcm.cpp:46: FAILED:
  REQUIRE( item->GetPayloadType() == payloadType )
with expansion:
  2 == 1

===============================================================================
test cases:   39 |   37 passed | 2 failed
assertions: 1600 | 1598 passed | 2 failed

So none of existing tests cover this LE/BE distinction yet.

I then decided to test the actual media flow with Rust echo example, but again on both branches it neither crashed nor produces any audio or video on receiving side. I guess actual s390x hardware is needed to test it properly or I missed something important or it is not going to work on BE platform without more testing and fixes 🤷

@ibc
Copy link
Member Author

ibc commented Aug 13, 2021

Wow amazing catch! Hi @jmillan (once you are back), look at this!

@xia-chu
Copy link

xia-chu commented Aug 16, 2021

Thank you for your work and excellent project.
When I was developing the zlmediakit project, I used part of the mediasoup code. Some users reported this problem when used zlmediakit.

This change makes sense to me

Thanks. Will wait a few days for @xia-chu.

@xia-chu
Copy link

xia-chu commented Aug 16, 2021

The zlmediakit project extracts some code from mediasoup, but retains the source description and mediasoup copyright notice. I hope this won't make you unhappy.

@xia-chu
Copy link

xia-chu commented Aug 16, 2021

This is where zlmediakit refers to the mediasoup source code

@ibc ibc merged commit 8fca7de into v3 Aug 16, 2021
@ibc ibc deleted the fix-size_t-to_int-pointer-conversion-in-64bits-big-endian-issue-636 branch August 16, 2021 13:42
@ibc
Copy link
Member Author

ibc commented Aug 16, 2021

The zlmediakit project extracts some code from mediasoup, but retains the source description and mediasoup copyright notice. I hope this won't make you unhappy.

Actually you reported a real issue and provided all the needed info, so of course... thanks!

Copy link
Member

@jmillan jmillan left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] srtp will crash on 64 bit big-endian machine
4 participants