Potential Bad memory access bug

[hnand]

Hi Erik,

I have written a self-contained test to display the problem we are seeing in our project that uses libsamplerate to do sample rate conversions. I have attached the entire libsamplerate directory along with my changes here. Additionally, I have also written up a brief description of the issue and my test below.
I would like to understand if this is indeed a potential bug or an improper use of the library. Looking forward to your thoughts.

Thanks,
Hari

Description:

Our project code follows the model in callback_test.c (ie: setup a callback using src_callback_new() with converter = SRC_SINC_FASTEST, numChannels = 32; calls to src_callback_read() till the requested # of samples are read).

We have discovered an intermittent issue where the audio data we receive after a call to src_callback_read() contains Nan values in the output buffer at a valid index (ie: within the total read samples returned by src_callback_read()). We traced this down to the calc_output_multi() function which is called by sinc_multichan_vari_process() in src_sinc.c. Specifically, in the scenarios where the issue happens, the data_index that calc_output_multi() calculates when applying the left half of the filter evaluates to a negative value (which leads to accessing uninitialized/unowned memory).


To simplify this, I have written a short self-contained test (called filterdata_integrity_test.c) under the tests directory. This test is similar to callback_test.c, ie: it sets up a callback using src_callback_new() with converter = SRC_SINC_FASTEST, numChannels = 2 (for simplicity); calls to src_callback_read() till the requested # of samples are read). The notable differences from my test and callback_test.c are:
i) Setting the src_ratio to a different value on every callback (by calling src_set_ratio()) -> this replicates the scenario in our project when the issue occurs
ii) Adding integrity checks on the audio data that is generated in the output buffer
All of the relevant changes also have comments next to them.

Note: I have also made an un-invasive change to src/src_sinc.c at line 437 to exit(1) whenever the data_index hits a negative value in calc_output_stereo() (since this is the only way I have found to catch the issue when it happens). I hit this exit every time I run the filterdata_integrity_test.

libsamplerate-0.1.8.zip

[erikd]

I managed to confirm this. Working on a fix, but it may only land when I get back from holidays next week.

[erikd]

The documentation suggests that setting the ratio using set_src_ratio is valid so this is definitely a bug in handling of some edge case.

[hnand]

Thank you for confirming, Erik.

This happens to be one of the real-world use case scenarios in our project. We can receive several playback rate change requests that need to be processed- and these translate to several src_set_ratio() calls during the course of one src_callback_read().

[erikd]

Managed to do some debugging work here and there, but still haven't got to the bottom of it. Should have a fix in the next week or so.

[hnand]

Hi Erik,

How is the fix for this bug looking? Any idea when you will be committing it?

Thanks,
Hari

[erikd]

Been crazy busy the last couple of weeks. This bug is a tough one. I need a couple of uninterrupted hours to debug this. Not sure when that will happen.

[Flamefire]

I tested this repro with the current version and it does not crash. However it silently buffer-underflows as the check is removed! This is a critical bug.

This is not even related to the callback interface and can be reproduced like this:

float data_in[256], output[20000];
SRC_STATE	*src_state;
SRC_DATA data;
int error;
src_state = src_new(converter, 2, &error);
data.data_in = data_in;
data.data_out = output;
data.input_frames = 128;
data.output_frames = 10000;
data.end_of_input = 0;
data.src_ratio = 0.09;
src_process(src_state, &data);
data.src_ratio = 0.08;
src_set_ratio(src_state, data.src_ratio);
src_process(src_state, &data);
src_delete(src_state);

I tracked this down to the following: The start of the buffer is filled with zeros in prepare_data by setting b_end==b_current to an offset calculated by the ratio. When reducing the ratio one would need more zeros, so a bigger offset. However this is only done once! So on the next call with a smaller ratio it will try to access the first element by calculating the index using b_current - offset, but offset is to big due to the smaller ratio resulting in a negative index.

Previously this negative index would call abort which is what @hnand reported. But now the filer->data[] array is directly accessed which will go back in the SINC_FILTER struct, first reading bogus data and for even higher ratio differences it would run outside the struct into other memory.

[Flamefire]

@hnand Your code will not do what you expect anyway. The problematic part is:

do {
	read_count = ARRAY_LEN (output) / test_callback_data.channels;
	read_count = src_callback_read (src_state, src_ratio, read_count, output);
	read_total += read_count;
}while (read_count > 0);

Inside the callback defined earlier you call src_set_ratio to change the current ratio. Only that you don't.

There are 2 problems:

• src_callback_read will call the callback until it is exhausted, aka returns a length of zero. Hence the loop will be run 2 times: The first time converting ALL data, the second doing nothing.
• src_set_ratio does not set the target ratio, but rather the current ratio. Inside the src_callback_read the regular src_process will be called with the current chunk returned by the callback. The ratio will then be interpolated over start to end of this chunk between the current ratio and the target ratio. The target ratio is the one passed to src_process which is here the same as the one in src_callback_read. So if you use a src_ratio of 2 and call src_set_ratio with 4 on each callback, where each callback gives 256 samples it would mean that you get (roughly) a ratio linearly decreasing from 4 to 2, then jump back to 4 after 256 input sample and decrease again.

@erikd Maybe a function like src_callback_read is needed which calls the callback exactly once before returning to allow use cases like the above? That won't solve the wrong ratios, but limit it at least. And/Or src_callback_read should query last_ratio before calling into src_process? Unless src_set_ratio is called from inside the callback this should not change the output.