Add an index check to prevent out of index #270
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #270 +/- ##
==========================================
+ Coverage 84.10% 84.12% +0.02%
==========================================
Files 24 24
Lines 1950 1953 +3
==========================================
+ Hits 1640 1643 +3
Misses 253 253
Partials 57 57
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@HyeockJinKim thank you for the fix! |
|
I added 2 test cases. 😄 |
|
Can I get a review again? @at-wat |
| res, err = pkt.Unmarshal(singlePayloadWithBrokenSecondNALU) | ||
| if err != nil { | ||
| t.Fatal(err) |
There was a problem hiding this comment.
Is it expected not to return error when tried to unmarshal a broken packet?
There was a problem hiding this comment.
When receiving the nalus of STAP-A, I was thinking of returning only the parsed nalus rather than returning an error because of the multiple nalus received.
When receiving a corrupted packet, is it better to return an error rather than returning the parsed nalus?
There was a problem hiding this comment.
I'm not very familiar with h264.
Is the result data usable even if some NALUs are missing?
Also, is it fine without notifying the caller that the packet is corrupted?
There was a problem hiding this comment.
In H.264, STAP-A packets can contain multiple nalu within an rtp packet. (A nalu can be an I Frame, P Frame, SPS, PPS, etc.).
If the H264 Unmarshal fails, returning an error seems to prevent decoding the wrong packet. (It also lets the end user know that there was a wrong packet.)
If it does not return an error, but only the parsed NALU, then if there were bad packets, it would not decode them anyway, and if the missing NALU was not essential for decoding, it would decode H264 packets.
In ffmpeg, which is used as a third_party in chromium, the STAP-A decode process parses as many nalus as it can, and then returns only the parsed nalus. Instead, it returns an error when not even a single nalu is parsed.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/ffmpeg/libavformat/rtpdec_h264.c;l=222-223?q=stap-a&ss=chromium
In my opinion, it would be nice if pion would decide how to handle this.
There was a problem hiding this comment.
I asked @Sean-Der on Slack to check this out
|
Sorry for the delay, this LGTM! I agree on returning the partial. It would be nice if we could log, but not available unfortunately. |
Description
In the current code,
binary.BigEndian.Uint16is causing an out of index panic while traversing the for loop.Add an length check to prevent panic from occurring.