Reuse rtp.Header in SessionSRTP.decrypt

[kcaffrey]

Decrypt used to allocate a new header for every inbound RTP packet, causing 1 or more allocations (extensions could cause several allocations due to slice growth, and CSRC could add one more). Reusing a header, something (*Context).decryptRTP() already supports, avoids that allocation.

rtp.Header supports reuse and will reuse the extension and CSRC slices when possible, causing per-packet allocations to drop to zero inside the srtp package.

Safety note: the header does not currently escape the srtp package to callers. The public (*Context).DecryptRTP() API seems to encourage (or at least allow) header reuse, so this change does not seem to add any new obligations. In fact, if somehow the header was retained or leaked to the caller, this would be a bug as the extension payloads alias the payload buffer that is already reused today.

Future work: srtp does still incur an allocation per incoming packet due to the replay detector. Removing this would likely involve a breaking API change in the replay detector.

Benchmark on all ciphers show a reduction from 2 allocs/op to 0 allocs/op (test packet had a single extension).

[kcaffrey]

One alternative I considered, and rejected, was having (streamSession).decrypt() take a *rtp.Header. The same interface, however, is also used for RTCP. I could pass both *rtp.Header AND *rtcp.Header, which would let the reused headers sit in the same goroutine and loop as the reused payload buffer. However, that seemed a bit too much of a kludge, and to add insult to injury, RTCP currently doesn't allocate for the *rtcp.Header as it doesn't escape.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.82%. Comparing base (ccffdeb) to head (261dbd3).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #378      +/-   ##
==========================================
+ Coverage   83.62%   83.82%   +0.20%     
==========================================
  Files          19       19              
  Lines        1484     1490       +6     
==========================================
+ Hits         1241     1249       +8     
+ Misses        135      134       -1     
+ Partials      108      107       -1     

Flag	Coverage Δ
go	83.82% <100.00%> (+0.20%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JoTurk]

We recently merged #372 to avoid some memory allocation before authentication, is there a way to make this optimization safe?

[kcaffrey]

We recently merged #372 to avoid some memory allocation before authentication, is there a way to make this optimization safe?

To make sure I understand, it appears that #372 is intended to prevent the memory footprint from growing due to things like malicious packets with spoofed ssrcs, so it discards any memory allocated unless the packet is authenticated. It does seem like allocations occur, but we are fine with that so long as they get garbage collected shortly after. And is the relation to this PR that since the header is reused, a malicious packet could cause the extension or CSRC slices within the reused header to balloon to large values? Or did you have other concerns?

Assuming that is your concern, it appears like the CSRC slice would be bounded at 16 elements no matter what is in the packet. The extension slice is a bit more complicated. The total number of extensions would be bounded more or less only by the size of the packet. The read loop uses an 8192 byte packet buffer, and each extension consumes at least two bytes, so I think extension count would be bounded at a little less than 4096 (so the slice would be at most ~100KB). This would be total, even when getting hammered by malicious packets, due to the header being reused. I don't think this exhibits the same unbounded per-SSRC growth that #372 fixed.

Please let me know if I missed something important though @JoTurk!

[kcaffrey]

If the ~100KB high water mark of memory is unacceptable, one alternative would be to restore the previous extension slice if decryption fails. Would you prefer that?

[kcaffrey]

@JoTurk I have amended the commit to include restoring the slices if decryption or unmarshalling fails, which I believe addresses the concerns you raised.

[JoTurk]

@kcaffrey I'm sorry I missed your comments, sounds good, feel free to undo the check if you want.