shithub: sox

Download patch

ref: 98e7e8e5157a39d540292c67f03cc523e1b7c650
parent: d96b541d0c4a9bfe42413a99e07691e249c2fc42
author: Ulrich Klauer <ulrich@chirlu.de>
date: Sun Oct 9 19:54:44 EDT 2011 

Prevent memory corruption in mcompand effect

As demonstrated by valgrind, mcompand did overrun its input buffer if
a delay was specified for at least one of its sub-compands, since only
the number of emitted output samples was counted, not the number of
consumed input samples. Possible consequences include data corruption
and segmentation faults.

This commit takes over some code from src/compand.c that does proper
counting of input and output samples, and aborts processing when they
diverge (which should mean that a delay was specified).

It seems to make mcompand work with delays - especially a different
delay for each band - would require a major overhaul. The best
(long-term) solution might be to replace mcompand with an effect that
only splits its input into bands (each band is one channel), then
compand each channel individually in an effect of its own.
Long-term. :)


--- a/src/mcompand.c
+++ b/src/mcompand.c
@@ -270,9 +270,10 @@
 
 static int sox_mcompand_flow_1(sox_effect_t * effp, priv_t * c, comp_band_t * l, const sox_sample_t *ibuf, sox_sample_t *obuf, size_t len, size_t filechans)
 {
-  size_t done, chan;
+  size_t idone, odone;
 
-  for (done = 0; done < len; ibuf += filechans) {
+  for (idone = 0, odone = 0; idone < len; ibuf += filechans) {
+    size_t chan;
 
     /* Maintain the volume fields by simulating a leaky pump circuit */
 
@@ -300,8 +301,8 @@
       if (c->delay_buf_size <= 0) {
         checkbuf = ibuf[chan] * level_out_lin;
         SOX_SAMPLE_CLIP_COUNT(checkbuf, effp->clips);
-        obuf[done++] = checkbuf;
-
+        obuf[odone++] = checkbuf;
+        idone++;
       } else {
         /* FIXME: note that this lookahead algorithm is really lame:
            the response to a peak is released before the peak
@@ -321,10 +322,14 @@
           SOX_SAMPLE_CLIP_COUNT(checkbuf, effp->clips);
           l->delay_buf[(l->delay_buf_ptr + c->delay_buf_size - l->delay_size)%c->delay_buf_size] = checkbuf;
         }
-        if (l->delay_buf_cnt >= c->delay_buf_size)
-          obuf[done++] = l->delay_buf[l->delay_buf_ptr];
-        else
+        if (l->delay_buf_cnt >= c->delay_buf_size) {
+          obuf[odone] = l->delay_buf[l->delay_buf_ptr];
+          odone++;
+          idone++;
+        } else {
           l->delay_buf_cnt++;
+          idone++; /* no "odone++" because we did not fill obuf[...] */
+        }
         l->delay_buf[l->delay_buf_ptr++] = ibuf[chan];
         l->delay_buf_ptr %= c->delay_buf_size;
       }
@@ -331,6 +336,16 @@
     }
   }
 
+  if (idone != odone || idone != len) {
+    /* Emergency brake - will lead to memory corruption otherwise since we
+       cannot report back to flow() how many samples were consumed/emitted.
+       Additionally, flow() doesn't know how to handle diverging
+       sub-compander delays. */
+    lsx_fail("Using a compander delay within mcompand is currently not supported");
+    exit(1);
+    /* FIXME */
+  }
+
   return (SOX_SUCCESS);
 }
 
@@ -354,6 +369,8 @@
     c->band_buf_len = len;
   }
 
+  len -= len % effp->out_signal.channels;
+
   ibuf_copy = lsx_malloc(*isamp * sizeof(sox_sample_t));
   memcpy(ibuf_copy, ibuf, *isamp * sizeof(sox_sample_t));
 
@@ -419,6 +436,8 @@
   size_t band, drained, mostdrained = 0;
   priv_t * c = (priv_t *)effp->priv;
   comp_band_t * l;
+
+  *osamp -= *osamp % effp->out_signal.channels;
 
   memset(obuf,0,*osamp * sizeof *obuf);
   for (band=0;band<c->nBands;++band) {