Re: [SoX-devel] [ sox-Bugs-3476843 ] Invalid memory access within flac handler after seek

[Eric W. <nor...@yh...>]

"SourceForge.net" <no...@so...> wrote:
> Bugs item #3476843, was opened at 2012-01-20 16:05
> Message generated for change (Comment added) made by eric_wong
> You can respond by visiting: 
> https://sourceforge.net/tracker/?func=detail&atid=110706&aid=3476843&group_id=10706

FYI, I strongly prefer comments via email, anyways...

> Comment By: eric_wong (eric_wong)
> Date: 2012-05-24 01:24
> 
> Message:
> I believe I have a fix for this issue, I can't figure out how to attach a
> patch, but you should be able to curl this to "git am"
> 
> http://bogomips.org/sox.git/patch/?id=d469441154331de90df5bd0366fe4fd07b65535a
> 
> You can also pull this from my git repo:
>   git pull git://bogomips.org/sox flac-mem-access-3476843

Just in case bogomips.org is down or unreachable:

>From d469441154331de90df5bd0366fe4fd07b65535a Mon Sep 17 00:00:00 2001
From: Eric Wong <nor...@yh...>
Date: Wed, 23 May 2012 21:52:28 +0000
Subject: [PATCH] flac: fix invalid memory access w/ optimize_trim

Reverse the buffer assignment roles and have read_samples() assign a
buffer location for decoder_write_callback().

Based on my analysis of src/libFLAC/stream_decoder.c in flac 1.2.1,
the previous SoX code to use an on-stack buffer was safe only (and
only questionably so) for non-seek decoding.

When seeking, FLAC__stream_decoder_seek_absolute() may call
FLAC__stream_decoder_process_single() internally to get to a target
sample.  In the seeking case, the write_audio_frame_to_client_()
function in libFLAC will use an on-stack array of pointers
("newbuffer", lines 2932-2938 of src/libFLAC/stream_decoder.c).

By stashing the address of newbuffer in p->decoded_wide_samples,
returning from FLAC__stream_decoder_seek_absolute() would leave
p->decoded_wide_samples pointing to the old stack location and
cause an invalid memory access.

ref: Invalid memory access within flac handler after seek - ID: 3476843
ref: https://sourceforge.net/tracker/?func=detail&aid=3476843&group_id=10706&atid=110706
---
 src/flac.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 106 insertions(+), 29 deletions(-)

diff --git a/src/flac.c b/src/flac.c
index 27c0bbf..452737a 100644
--- a/src/flac.c
+++ b/src/flac.c
@@ -35,9 +35,10 @@ typedef struct {
   uint64_t total_samples;
 
   /* Decode buffer: */
-  FLAC__int32 const * const * decoded_wide_samples;
-  unsigned number_of_wide_samples;
-  unsigned wide_sample_number;
+  sox_sample_t *req_buffer; /* this may be on the stack */
+  size_t number_of_requested_samples;
+  sox_sample_t *leftover_buf; /* heap */
+  unsigned number_of_leftover_samples;
 
   FLAC__StreamDecoder * decoder;
   FLAC__bool eof;
@@ -158,6 +159,11 @@ static FLAC__StreamDecoderWriteStatus decoder_write_callback(FLAC__StreamDecoder
 {
   sox_format_t * ft = (sox_format_t *) client_data;
   priv_t * p = (priv_t *)ft->priv;
+  sox_sample_t * dst = p->req_buffer;
+  unsigned channel;
+  unsigned nsamples = frame->header.blocksize;
+  unsigned sample = 0;
+  size_t actual = nsamples * p->channels;
 
   (void) flac;
 
@@ -165,11 +171,47 @@ static FLAC__StreamDecoderWriteStatus decoder_write_callback(FLAC__StreamDecoder
     lsx_fail_errno(ft, SOX_EINVAL, "FLAC ERROR: parameters differ between frame and header");
     return FLAC__STREAM_DECODER_WRITE_STATUS_ABORT;
   }
+  if (dst == NULL) {
+    lsx_warn("FLAC ERROR: entered write callback without a buffer (SoX bug)");
+    return FLAC__STREAM_DECODER_WRITE_STATUS_ABORT;
+  }
+
+  /* FLAC may give us too much data, prepare the leftover buffer */
+  if (actual > p->number_of_requested_samples) {
+    size_t to_stash = actual - p->number_of_requested_samples;
+
+    p->leftover_buf = lsx_malloc(to_stash * sizeof(sox_sample_t));
+    p->number_of_leftover_samples = to_stash;
+    nsamples = p->number_of_requested_samples / p->channels;
+
+    p->req_buffer += p->number_of_requested_samples;
+    p->number_of_requested_samples = 0;
+  } else {
+    p->req_buffer += actual;
+    p->number_of_requested_samples -= actual;
+  }
+
+leftover_copy:
+
+  for (; sample < nsamples; sample++) {
+    for (channel = 0; channel < p->channels; channel++) {
+      FLAC__int32 d = buffer[channel][sample];
+      switch (p->bits_per_sample) {
+      case  8: *dst++ = SOX_SIGNED_8BIT_TO_SAMPLE(d,); break;
+      case 16: *dst++ = SOX_SIGNED_16BIT_TO_SAMPLE(d,); break;
+      case 24: *dst++ = SOX_SIGNED_24BIT_TO_SAMPLE(d,); break;
+      case 32: *dst++ = SOX_SIGNED_32BIT_TO_SAMPLE(d,); break;
+      }
+    }
+  }
+
+  /* copy into the leftover buffer if we've prepared it */
+  if (sample < frame->header.blocksize) {
+    nsamples = frame->header.blocksize;
+    dst = p->leftover_buf;
+    goto leftover_copy;
+  }
 
-  /* FIXME: We're skating on thin ice here: buffer may be on the stack! */
-  p->decoded_wide_samples = buffer;
-  p->number_of_wide_samples = frame->header.blocksize;
-  p->wide_sample_number = 0;
   return FLAC__STREAM_DECODER_WRITE_STATUS_CONTINUE;
 }
 
@@ -224,35 +266,66 @@ static int start_read(sox_format_t * const ft)
 static size_t read_samples(sox_format_t * const ft, sox_sample_t * sampleBuffer, size_t const requested)
 {
   priv_t * p = (priv_t *)ft->priv;
-  size_t actual = 0;
+  size_t prev_requested;
 
   if (p->seek_pending) {
     p->seek_pending = sox_false; 
-    p->wide_sample_number = p->number_of_wide_samples = 0;
-    if (!FLAC__stream_decoder_seek_absolute(p->decoder, (FLAC__uint64)(p->seek_offset / ft->signal.channels)))
+
+    /* discard leftover decoded data */
+    free(p->leftover_buf);
+    p->leftover_buf = NULL;
+    p->number_of_leftover_samples = 0;
+
+    p->req_buffer = sampleBuffer;
+    p->number_of_requested_samples = requested;
+
+    /* calls decoder_write_callback */
+    if (!FLAC__stream_decoder_seek_absolute(p->decoder, (FLAC__uint64)(p->seek_offset / ft->signal.channels))) {
+      p->req_buffer = NULL;
       return 0;
+    }
+  } else if (p->number_of_leftover_samples > 0) {
+
+    /* small request, no need to decode more samples since we have leftovers */
+    if (requested < p->number_of_leftover_samples) {
+      size_t req_bytes = requested * sizeof(sox_sample_t);
+
+      memcpy(sampleBuffer, p->leftover_buf, req_bytes);
+      p->number_of_leftover_samples -= requested;
+      memmove(p->leftover_buf, (char *)p->leftover_buf + req_bytes,
+              (size_t)p->number_of_leftover_samples * sizeof(sox_sample_t));
+      return requested;
+    }
+
+    /* first, give them all of our leftover data: */
+    memcpy(sampleBuffer, p->leftover_buf,
+           p->number_of_leftover_samples * sizeof(sox_sample_t));
+
+    p->req_buffer = sampleBuffer + p->number_of_leftover_samples;
+    p->number_of_requested_samples = requested - p->number_of_leftover_samples;
+
+    free(p->leftover_buf);
+    p->leftover_buf = NULL;
+    p->number_of_leftover_samples = 0;
+
+    /* continue invoking decoder below */
+  } else {
+    p->req_buffer = sampleBuffer;
+    p->number_of_requested_samples = requested;
   }
-  while (!p->eof && actual < requested) {
-    if (p->wide_sample_number >= p->number_of_wide_samples)
-      FLAC__stream_decoder_process_single(p->decoder);
-    if (p->wide_sample_number >= p->number_of_wide_samples)
+
+  /* invoke the decoder, calls decoder_write_callback */
+  while ((prev_requested = p->number_of_requested_samples) && !p->eof) {
+    if (!FLAC__stream_decoder_process_single(p->decoder))
+      break; /* error, but maybe got earlier in the loop, though */
+
+    /* number_of_requested_samples decrements as the decoder progresses */
+    if (p->number_of_requested_samples == prev_requested)
       p->eof = sox_true;
-    else { /* FIXME: this block should really be inside the decode callback */
-      unsigned channel;
-
-      for (channel = 0; channel < p->channels; channel++, actual++) {
-        FLAC__int32 d = p->decoded_wide_samples[channel][p->wide_sample_number];
-        switch (p->bits_per_sample) {
-        case  8: *sampleBuffer++ = SOX_SIGNED_8BIT_TO_SAMPLE(d,); break;
-        case 16: *sampleBuffer++ = SOX_SIGNED_16BIT_TO_SAMPLE(d,); break;
-        case 24: *sampleBuffer++ = SOX_SIGNED_24BIT_TO_SAMPLE(d,); break;
-        case 32: *sampleBuffer++ = SOX_SIGNED_32BIT_TO_SAMPLE(d,); break;
-        }
-      }
-      ++p->wide_sample_number;
-    }
   }
-  return actual;
+  p->req_buffer = NULL;
+
+  return requested - p->number_of_requested_samples;
 }
 
 
@@ -263,6 +336,10 @@ static int stop_read(sox_format_t * const ft)
   if (!FLAC__stream_decoder_finish(p->decoder) && p->eof)
     lsx_warn("decoder MD5 checksum mismatch.");
   FLAC__stream_decoder_delete(p->decoder);
+
+  free(p->leftover_buf);
+  p->leftover_buf = NULL;
+  p->number_of_leftover_samples = 0;
   return SOX_SUCCESS;
 }
 
-- 
Eric Wong