Repository navigation
Harden the audio decoder against bad packets and a closed stream - #127
Conversation
PR Summary by QodoHarden RTP audio decoding and stream shutdown
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit 9011e99 |
josephnef
left a comment
josephnef
left a comment
There was a problem hiding this comment.
This is a good set of fixes and I want it in. The six defects are real: the 250-byte fixed copy, the negative payload size feeding a VLA, runt RTP packets reaching the parser, the write-after-close on the AAudio stream, delete on a malloc'd Opus decoder, and decoding under the queue lock. The replacements are correct on my read: the RTP header walk checks header + 4 before reading the extension length, padding is validated against the payload, m_pcm is sized for the longest legal Opus packet, the queue is bounded with oldest-drop, and m_mtxStream covers both the decode-thread write and the close. The follow-ups to Qodo's two findings in 9011e99 are right as well.
One blocking item, mechanical: the clang-format job fails on this branch. app/videonative has its own .clang-format, and clang-format 18.1.8 wants 19 lines changed in AudioDecoder.cpp, 8 in AudioDecoder.h, 7 in VideoPlayer.cpp. pip install clang-format==18.1.8 && scripts/format_sources.sh and push; the rest of CI (host test, debug APK, emulator smoke) is already green on the reopened run.
Minor, take or leave: kMaxQueued = 50 at 20 ms frames is a second of audio; fine for live playback, just worth a comment that it is deliberate latency bounding, which you have.
Audio was checked end to end on a Quest 3 against an OpenIPC camera with audio switched on in majestic (audio.enabled true, Opus): the camera's microphone, Opus in the RTP stream over wfb, decoded here and played in the headset. It works. Reading the decoder on the way turned up six defects, most of them reachable from radio input. - The packet copy wrote into a fixed 250-byte array with no length check. That does not even take malformed input: 20 ms of PCM at 8 kHz is already 320 bytes, so majestic's pcm codec can write past the end with perfectly ordinary packets. The buffer holds 1500 bytes now and anything larger is dropped - dropped, not truncated, because a cut Opus packet decodes to noise rather than failing. - The payload was read from a fixed offset of 12 with no check. A shorter packet gave a negative size; opus_packet_get_nb_frames() then returned an error that was not checked; and the product sized a variable-length array on the stack. Decoding goes into a fixed buffer for the longest packet Opus allows - 120 ms, 5760 samples at 48 kHz - and the header length is worked out from the packet, CSRCs, extension and padding included. - RTPPacket reads its header straight out of the buffer and its length assert is commented out, so a runt packet was an out-of-bounds read with an underflowed payload size handed to both the video parser and the audio decoder. Every RTP packet enters through onNewRTPData(), which now drops anything shorter than a header; the payload size is clamped in RTPPacket as well. - The stream could be written to after it was closed. VideoActivity.onPause() calls VideoPlayer.stop(), which closes the stream, before stopAudio() has stopped the decode thread, so that thread could still write to it. stopAudio() also left the pointer set, so the destructor closed the same stream a second time. The stream is behind its own mutex now and cleared when closed. - The Opus decoder comes from opus_decoder_create(), which allocates with malloc, and was freed with delete. It is destroyed with opus_decoder_destroy() now. A start after stop also created a second decoder without freeing the first - and onResume() is such a start, after every pause. - Packets were decoded with the queue lock held, so enqueueAudio() on the receive thread - which carries the video too - waited out every decode. The lock is released before decoding. initAudio() checked none of the calls it made and reported success regardless; failures are logged and reported now.
Two points from review, both fair. Decoding now runs with the queue lock released, which was the point - the receive thread, which carries the video too, no longer waits out every decode. But that lock was also, by accident, the only thing holding the receive thread back, and the queue itself has never had a limit. If the decoder ever fell behind, packets would pile up without bound. The queue holds at most 50 packets now, about a second of 20 ms frames, and when it is full the oldest goes: for live audio a packet played late is only latency. The 1500-byte limit was arbitrary. The receivers hand over up to 65507 bytes (UdpReceiver's UDP_PACKET_MAX_SIZE; UdsReceiver stops at 3700), so a packet the link could legitimately deliver was being dropped. Each packet is stored at its own size now, and only something longer than either receiver can produce is refused.
What scripts/format_sources.sh produces with the version CI pins, and nothing else. The branch was written before the format check existed; VideoPlayer.cpp needed nothing once it was rebased onto the tree OpenIPC#131 formatted.
From review: with the decode running outside the queue lock, packets can be waiting when audio stops, and the next start played them before anything new. Whatever the decode thread had not reached when it was stopped is cleared once it has been joined. nativeStartAudio() stops before it starts, so every start begins with an empty queue. Checked on a desktop against stubbed AAudio and Opus, with a decoder held up while ten packets queued behind it: before this change the nine waiting packets were all decoded after the restart, ahead of the fresh one; now only the fresh one is.
9011e99 to
e920502
Compare
|
@josephnef Thanks for the careful read. Rebased onto current master (a68a61f) and formatted with Qodo's second finding was real: audio still queued when the decoder stopped was played after the next start. Fixed in e920502, details in that thread.
|
josephnef
left a comment
josephnef
left a comment
There was a problem hiding this comment.
Approving. Checked the new head e920502 against the previously reviewed 9011e99 with whitespace ignored: the only non-format change is the queue clear in stopAudioProcessing(), which sits after the thread join so it cannot contend with the decode thread, and it correctly stops pre-stop audio from being the first thing played after a restart. Rebase is on current master. CI is green on this head: clang-format, host GoogleTest, debug APK and the emulator smoke test.
I checked audio end to end on a Quest 3 against an OpenIPC camera (SSC338Q, OpenIPC 2.6) with
audio switched on in majestic: the camera's microphone, Opus in the RTP stream over wfb-ng,
decoded here and played in the headset. It works. Reading
AudioDecoderon the way turned upsix defects, most of them reachable from radio input.
The decoder
The packet copy overflowed on ordinary packets.
AudioUDPPacketcopied into a fixed250-byte array with no length check. That does not even take malformed input: 20 ms of PCM at
8 kHz is already 320 bytes, so majestic's
pcmcodec can write past the end with perfectlyordinary packets. Each packet is stored at its own size now, up to the 65507 bytes the UDP
receiver can hand over, and only something longer is refused — refused, not truncated, because a
cut Opus packet decodes to noise rather than failing.
A short packet sized a stack array from a negative number. The payload was read from a fixed
offset of 12 with no check. A shorter packet made the size negative,
opus_packet_get_nb_frames()then returned an error that was not checked, and the product sizeda variable-length array on the stack. Decoding goes into a fixed buffer for the longest packet
Opus allows — 120 ms, 5760 samples at 48 kHz — and the header length is worked out from the
packet itself, CSRCs, extension and padding included.
Runt packets reached the parser.
RTPPacketreads its header straight out of the buffer,and its length assert is commented out, so a packet shorter than a header was an out-of-bounds
read with an underflowed payload size handed to both the video parser and the audio decoder.
Every RTP packet enters through
onNewRTPData(), which now drops anything shorter than a header;the payload size is clamped in
RTPPacketas well.The stream could be written to after it was closed.
VideoActivity.onPause()callsVideoPlayer.stop(), which closes the stream, beforestopAudio()has stopped the decodethread — so that thread could still write to it.
stopAudio()also left the pointer set, so thedestructor closed the same stream a second time. The stream is behind its own mutex now and
cleared when closed.
The Opus decoder was freed with
delete. It comes fromopus_decoder_create(), whichallocates with
malloc; it is released withopus_decoder_destroy()now. A start after a stopalso created a second decoder without freeing the first — and
onResume()is such a start,after every pause.
The receive thread waited for every decode. Packets were decoded with the queue lock held, so
enqueueAudio()— called on the receive thread, which carries the video too — waited out eachdecode. The lock is released before decoding. That lock was also, by accident, the only thing
holding the receive thread back, so the queue is bounded now: at most 50 packets, about a second,
oldest dropped first. Whatever is still queued when audio stops is cleared, so a restart does not
begin with stale audio.
initAudio()checked none of the calls it made and reported success regardless; failures arelogged and reported now.
Testing
On a Quest 3 with the camera above: audio and video play on this branch, and the same process
went through several pause and resume cycles — each one closes the stream in
onPause()andopens a new one in
onResume(), which is where the leaked decoder and the write to a closedstream used to happen — with nothing in the crash buffer. That build also carried #126, merged
since; the two touch different files.
Clearing the queue on stop came later, from review, and has not been on the headset yet. It was
checked on a desktop build of
AudioDecoderagainst stubbed AAudio and Opus: with a decode heldup and packets queued behind it, every waiting packet used to be decoded after a stop and start;
now none is.
The packet-handling fixes were found by reading the code and are bounds checks; I have not
driven the decoder with crafted packets.
The branch is on current master and clean under clang-format 18.1.8 (
scripts/format_sources.sh).