AmAudio/AmAudioFile: bound read size against the sample buffer

[hecko]

The bug

AmAudio::get() computes how much to read as:

int size = calcBytesToRead((int)((float)nb_samples * (float)getSampleRate()
                                 / (float)output_sample_rate));
size = read(rd_ts, size);
...
memcpy(buffer, (unsigned char*)samples, size);

size is never checked against anything before it is used:

• read() fills the samples DblBuffer, which only ever exposes AUDIO_BUFFER_SIZE (8192) bytes at a time — unsigned char samples[AUDIO_BUFFER_SIZE * 2] with half live.
• the result is memcpy()'d into the caller's buffer, which is also AUDIO_BUFFER_SIZE (AmMediaProcessorThread::buffer[AUDIO_BUFFER_SIZE], passed down through readStreams()).

Both getSampleRate() (the negotiated remote codec rate) and output_sample_rate (the other leg's rate) come from SDP. A skewed ratio between them — a high-rate codec feeding a low-rate output leg — scales the request up without bound, and a negative intermediate from the float→int conversion is passed straight through. The overrun lands in AmAudio's own object memory and in the media processor thread's buffer.

AmAudioFile::read() has the same exposure one level down: it fread()s size bytes into the same DblBuffer with no bound check of its own.

The fix

Reject oversized (and negative) requests in both places rather than overrunning.

AUDIO_BUFFER_SIZE is 8192 bytes — several hundred milliseconds of PCM16 at any rate SEMS negotiates — so no legitimate frame is affected. A typical 20 ms G.711 frame is 320 bytes.

This complements the existing zero-clock-rate rejections (#541, #542, #495) by bounding the upper end of the same computation.

Validation

Full cmake build of core and all apps: clean, no new warnings.

Credit

Backported from sipwise/sems commit c6a33f41 ("AmAudio/AmAudioFile: add buffer guards (overflow)", MT#65557). Thanks to the Sipwise team for the original fix; adapted to this tree (which still uses the plain int return path in AmAudio::get()).

---

Generated by Claude Code

[hecko]

deb_test_build is failing here, but not because of this change.

It dies in Dockerfile-debian11 at the very first RUN apt update, before any source is compiled:

E: Release file for http://deb.debian.org/debian-security/dists/bullseye-security/InRelease
   is expired (invalid since 4d 6h 8min 32s)
ERROR: failed to solve: process "/bin/sh -c apt update" did not complete successfully: exit code: 100

Debian 11 (bullseye) reached end of security support, so its InRelease file is no longer being refreshed and apt now rejects it as expired. The same job is already red on master at e5e7e61 for the same reason (run 34616001093), and this PR touches only core/AmAudio.cpp and core/AmAudioFile.cpp.

There is no fix for it in the tree yet to carry into this PR. Unblocking it needs a packaging-side change (bump the base image off bullseye, or pass -o Acquire::Check-Valid-Until=false in that Dockerfile), which belongs in its own PR rather than here.

The compile itself is fine: a full cmake build of core plus all apps is clean with no new warnings, and the remaining checks (hardened/asan/ubsan/tsan, FreeBSD 13.5/14.3/15.0, macOS, rpm) are running.

---

Generated by Claude Code

[Copilot]

🟡 Changes recommended

Two critical buffer-overrun paths remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request adds bounds checks for audio reads to protect fixed-size buffers from malformed sample-rate ratios.

Changes:

• Bounds computed reads in AmAudio::get().
• Bounds file reads before fread().

Review findings:

• core/AmAudio.cpp:312 — Critical (3 votes): validate the scaled sample count before conversion to int and passing it to calcBytesToRead.
• core/AmAudioFile.cpp:381 — Critical (1 vote): account for decoder output expansion after reading.

File summaries

File	Summary
core/AmAudioFile.cpp	Adds file-read bounds; decoder output can still exceed the buffer.
core/AmAudio.cpp	Adds computed-read validation; validation occurs too late.

Review details

Suppressed comments (3)

core/AmAudio.cpp:312

• This only bounds the pre-read size; the later decode() and resampleOutput() can expand that data before line 338 copies it. In particular, when the input rate is lower than output_sample_rate, the resampler writes/returns more than AUDIO_BUFFER_SIZE without receiving a destination capacity, so samples and the caller's fixed buffer can still be overrun. Reject or otherwise capacity-check the final PCM size before invoking the resampler.

  if(size < 0 || (unsigned int)size > AUDIO_BUFFER_SIZE){

core/AmAudio.cpp:312

• AmRtpAudio::get() overrides the base method, so it never reaches this guard. It performs the same SDP-controlled rate scaling and passes the resulting sample count to playout_buffer->read() before copying to buffer; the B2B path calls this with the destination leg's rate, so a high-rate source/low-rate output can still write past the samples DblBuffer. Add an equivalent bound to the override or centralize the bounded read path.

  if(size < 0 || (unsigned int)size > AUDIO_BUFFER_SIZE){

core/AmAudio.cpp:318

• Checking the encoded read() size here does not bound what decode() writes into the back half of samples. For example, the PCMU/PCMA decoders write size * 2 PCM bytes, so an input size of exactly 8192 passes this check but AmAudio::decode() writes 16384 bytes into an 8192-byte half-buffer. The limit must account for decoder expansion before calling decode().

  if(size < 0 || (unsigned int)size > AUDIO_BUFFER_SIZE){
    ERROR("AmAudio::get: refusing read of %i bytes (max %i): nb_samples=%u, "
	  "input_rate=%i, output_rate=%i\n",
	  size, AUDIO_BUFFER_SIZE, nb_samples, getSampleRate(),
	  output_sample_rate);
    return -1;
  }

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.