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

[core] Fix crash case in FEC filter #891

Merged
merged 1 commit into from Oct 22, 2019
Merged

[core] Fix crash case in FEC filter #891

merged 1 commit into from Oct 22, 2019

Conversation

sorayuki
Copy link
Contributor

@sorayuki sorayuki commented Sep 28, 2019

I met some crash when playing with this library.

FEC filter may access item out of range in rcv.cells.
FEC filter may access item out of range in rcv.rowq.

@sorayuki sorayuki changed the title [core] Fix some crash case [core] Fix crash case in FEC filter and Congestion Control Oct 2, 2019
@sorayuki
Copy link
Contributor Author

sorayuki commented Oct 2, 2019

I don't know why appveyor CI report the branch as non-mergeable. Github says there is no conflicts.
It does fix the crash bugs; could I have my pull request reviewed?

@ethouris
Copy link
Collaborator

ethouris commented Oct 2, 2019

There's some problem with AppVeyor for now. Don't worry about it, your change isn't going to raise any portability issues.

However, there was a different fix for the crash in congestion control already provided; please upgrade to the latest master. The fix was also a little different.

@sorayuki sorayuki changed the title [core] Fix crash case in FEC filter and Congestion Control [core] Fix crash case in FEC filter Oct 2, 2019
@sorayuki
Copy link
Contributor Author

sorayuki commented Oct 2, 2019

done

@ethouris
Copy link
Collaborator

ethouris commented Oct 2, 2019

I'd have a question about line 1712, introduced new condition.

If this is going to generate an IPE then it's only a debug support, and this condition shouldn't have been satisfied in a non-buggy software (should be prevented from much earlier).

Have you found a case when you can see this log?

@sorayuki
Copy link
Contributor Author

sorayuki commented Oct 2, 2019

case:
[curl] - [srt tunnel] - [fujian, china] - [internet] - [tokyo, japan] - [srt tunnel] - [gost proxy]

srt socket runs in SRTT_LIVE mode which is not suitable for the http tunnel case, with FEC filter.

These are screenshot of crash report in Visual Studio 2019,
and the network traffic capture,
and the variable values when crash,
and the options of srt socket.

pr891-crash.zip

variables

@maxsharabayko maxsharabayko added [core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior labels Oct 4, 2019
@maxsharabayko maxsharabayko added this to Needs review in Development via automation Oct 4, 2019
@maxsharabayko maxsharabayko added this to the v1.4.1 milestone Oct 4, 2019
Development automation moved this from Needs review to Reviewer approved Oct 21, 2019
@rndi rndi merged commit 3d5ffe2 into Haivision:master Oct 22, 2019
Development automation moved this from Reviewer approved to Done Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
No open projects
Development
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants