Skip to content

Fix race in rtp.Session. - #81

Merged
alexfish8 merged 3 commits into
mainfrom
afish/fix-race
Aug 24, 2026
Merged

Fix race in rtp.Session.#81
alexfish8 merged 3 commits into
mainfrom
afish/fix-race

Conversation

@alexfish8

@alexfish8 alexfish8 commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

There's a few race/race-like conditions in the current implementation of rtp.readStream:

  1. For a given reader, and for a given order of packets written to rtp.readStream, it's possible for the packets to be read out of order, e.g.:
  • writer writes packet A to the stream, and it lands in the queue (since the stream hasn't been lent a buffer yet)
  • reader calls ReadRTP and it lends its buffer to readStream. the reader then parks itself on the direct select statement.
  • writer writes packet B to the stream, and it gets written to the borrowed buffer
  • in the select statement, the reader receives packet A if c.recv is selected, or packet B if c.copied is selected. If packet A is received, then packet B is dropped when the defer statement is run (that nils out the lent buffer and header. If packet B is received, then there's an out-of-order receive.

This PR fixes the issue by ensuring a few things:

  1. Whenever we attempt to read, always check to see if there's any queued packets first.
  2. Hold the lock whenever:
    a. we write a packet to the stream
    b. we check to see if the reader should lend its buffer or not.

Also, add a test.

@boks1971 boks1971 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

good catch!

Comment thread rtp/session.go
if r.hdr != nil {
// zero copy
*r.hdr = p.Header
n := copy(r.payload, p.Payload)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

off-topic, why is this called zero-copy?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, I was wondering the same thing, actually.

@alexfish8
alexfish8 merged commit c8704a7 into main Aug 24, 2026
5 checks passed
@alexfish8
alexfish8 deleted the afish/fix-race branch August 24, 2026 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants