-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Catch v2.0.1: TestRtpStreamRecv.cpp: SIGSEGV #173
Comments
NOTE: I've upgraded libuv to 1.18.0 and Catch to 2.0.1, but I don't think this is related at all: |
Opss! it does not fail with Catch 1.9.7 (the version until today) neither it fails with 1.11.0 (the latest 1.X release), so it's definitely related to some change in Catch v2.0.1 (that just happens in OSX!). Here the list of breaking changes: https://github.com/catchorg/Catch2/releases/tag/v2.0.1 NOTE: Downgrading to 1.11.0 until we discover the problem. |
It's very interesting that, in OSX with Catch 2.0.1, just the following tests fail:
|
Opss, in TestVP8.cpp, if I rename So I think this is the same error we already had: variables are shared across all the tests, specially in OSX. Well, not exactly, here the problem is that NOTE: I was not able to make the TestRtpStreamRecv failing test work with any hack as variable renaming, so no idea here... |
Reported in Catch: catchorg/Catch2#1118 |
Before worker/src/RTC/Codecs/VP8.cpp#L96, offset was incremented twice ( |
Oh, thanks @lightmare, will check it. |
The TestVP8.cpp issue has been fixed. As @lightmare commented it was accessing a position of the buffer that was not initialised. |
Well that fixed the test sample, but the function |
You're right. Fixed in ed28b24. Thanks @lightmare. |
It happens in OSX. It does NOT happen in Linux.
The core dump:
It points to this line:
uint16_t seq = nackInfo.seq;
so it seems that
nackInfo
was not what it was supposed to be...The text was updated successfully, but these errors were encountered: