xiph / rnnoise

Recurrent neural network for audio noise reduction
BSD 3-Clause "New" or "Revised" License
3.89k stars 882 forks source link

Fix a memory leak in denoise.c #182

Closed a-rose closed 3 years ago

a-rose commented 3 years ago

Using valgrind has revelead 3 small leaks, let's get this cleaned up :)

jmvalin commented 3 years ago

That "fix" is dangerous. The fft state is for the whole library, so it's not a real leak. But your patch frees it for the decoder state, so if you: 1) init a state 2) destroy a state 3) init another state 4) use that new state then you'll get a use-after-free

a-rose commented 3 years ago

I noticed I forgot to reset common.init after freeing everything, this will definitely cause a use-after-free error. What a dumb mistake. 😅

Is this what you meant, or is there anything else making this "dangerous" ? common is only in denoise.c and everytime it is used, check_init() ensures kfft is allocated. After resetting common.init in rnnoise_destroy, something like this works fine:

// based on rnnoise_demo.c

void run(DenoiseState *st, FILE *f1, FILE *fout) {  int i;
  int first = 1;
  float x[FRAME_SIZE];

  while (1) {
    short tmp[FRAME_SIZE];
    fread(tmp, sizeof(short), FRAME_SIZE, f1);
    if (feof(f1)) break;
    for (i=0;i<FRAME_SIZE;i++) x[i] = tmp[i];
    rnnoise_process_frame(st, x, x);
    for (i=0;i<FRAME_SIZE;i++) tmp[i] = x[i];
    if (!first) fwrite(tmp, sizeof(short), FRAME_SIZE, fout);
    first = 0;
  }
}

int main(int argc, char **argv) {
...
  st = rnnoise_create(NULL);
  run(st, f1, fout);
  rnnoise_destroy(st);

  fseek(f1, 0, SEEK_SET);

  st = rnnoise_create(NULL);
  run(st, f1, fout);
  rnnoise_destroy(st);
  ...
}