Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle memory allocation failures, and reduce overflow #4

Closed
wants to merge 2 commits into from
Closed

Handle memory allocation failures, and reduce overflow #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2aa499b
Handle memory allocation failures during initialization
Apr 19, 2016
153d26e
Reduce likelihood of overflow and handle arithmetic overflow gracefully.
May 7, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions include/speex/speex_resampler.h
View file
Open in desktop
Expand Up @@ -104,6 +104,7 @@ enum {
RESAMPLER_ERR_BAD_STATE = 2,
RESAMPLER_ERR_INVALID_ARG = 3,
RESAMPLER_ERR_PTR_OVERLAP = 4,
RESAMPLER_ERR_OVERFLOW = 5,

RESAMPLER_ERR_MAX_ERROR
};
Expand Down
75 changes: 39 additions & 36 deletions libspeexdsp/resample.c
View file
Open in desktop
Expand Up @@ -90,6 +90,10 @@ static void speex_free (void *ptr) {free(ptr);}
#define NULL 0
#endif

#ifndef UINT32_MAX
#define UINT32_MAX 4294967296U
#endif

#ifdef _USE_SSE
#include "resample_sse.h"
#endif
Expand Down Expand Up @@ -585,6 +589,19 @@ static int resampler_basic_zero(SpeexResamplerState *st, spx_uint32_t channel_in
return out_sample;
}

static int _muldiv(spx_uint32_t *result, spx_uint32_t value, spx_uint32_t mul, spx_uint32_t div)
{
spx_uint32_t major = value / div;
spx_uint32_t remainder = value % div;
/* TODO: Could use 64 bits operation to check for overflow. But only guaranteed in C99+ */
if (remainder > UINT32_MAX / mul || major > UINT32_MAX / mul
|| major * mul > UINT32_MAX - remainder * mul / div)
return RESAMPLER_ERR_OVERFLOW;
if (result)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a static function I don't think this check is necessary, you could do speex_assert(result) at the beginning of this function if you want to be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I feel like you're now stretching my good will. You are free to use this pull request as it is, or modify it as you want. But be aware that that it has been classified as sec-critical.
Seeing that it's been over 3 weeks since I first submitted handling out of memory errors and it has yet to be included, I believe my job is done now.

*result = remainder * mul / div + major * mul;
return RESAMPLER_ERR_SUCCESS;
}

static int update_filter(SpeexResamplerState *st)
{
spx_uint32_t old_length = st->filt_len;
Expand All @@ -602,8 +619,8 @@ static int update_filter(SpeexResamplerState *st)
{
/* down-sampling */
st->cutoff = quality_map[st->quality].downsample_bandwidth * st->den_rate / st->num_rate;
/* FIXME: divide the numerator and denominator by a certain amount if they're too large */
st->filt_len = st->filt_len*st->num_rate / st->den_rate;
if (_muldiv(&st->filt_len,st->filt_len,st->num_rate,st->den_rate) != RESAMPLER_ERR_SUCCESS)
goto fail;
/* Round up to make sure we have a multiple of 8 for SSE */
st->filt_len = ((st->filt_len-1)&(~0x7))+8;
if (2*st->den_rate < st->num_rate)
Expand Down Expand Up @@ -792,6 +809,12 @@ EXPORT SpeexResamplerState *speex_resampler_init_frac(spx_uint32_t nb_channels,
return NULL;
}
st = (SpeexResamplerState *)speex_alloc(sizeof(SpeexResamplerState));
if (!st)
{
if (err)
*err = RESAMPLER_ERR_ALLOC_FAILED;
return NULL;
}
st->initialised = 0;
st->started = 0;
st->in_rate = 0;
Expand All @@ -813,9 +836,12 @@ EXPORT SpeexResamplerState *speex_resampler_init_frac(spx_uint32_t nb_channels,
st->buffer_size = 160;

/* Per channel data */
st->last_sample = (spx_int32_t*)speex_alloc(nb_channels*sizeof(spx_int32_t));
st->magic_samples = (spx_uint32_t*)speex_alloc(nb_channels*sizeof(spx_uint32_t));
st->samp_frac_num = (spx_uint32_t*)speex_alloc(nb_channels*sizeof(spx_uint32_t));
if (!(st->last_sample = (spx_int32_t*)speex_alloc(nb_channels*sizeof(spx_int32_t))))
goto fail;
if (!(st->magic_samples = (spx_uint32_t*)speex_alloc(nb_channels*sizeof(spx_uint32_t))))
goto fail;
if (!(st->samp_frac_num = (spx_uint32_t*)speex_alloc(nb_channels*sizeof(spx_uint32_t))))
goto fail;
for (i=0;i<nb_channels;i++)
{
st->last_sample[i] = 0;
Expand All @@ -838,6 +864,12 @@ EXPORT SpeexResamplerState *speex_resampler_init_frac(spx_uint32_t nb_channels,
*err = filter_err;

return st;

fail:
if (err)
*err = RESAMPLER_ERR_ALLOC_FAILED;
speex_resampler_destroy(st);
return NULL;
}

EXPORT void speex_resampler_destroy(SpeexResamplerState *st)
Expand Down Expand Up @@ -1080,36 +1112,6 @@ static inline spx_uint32_t _gcd(spx_uint32_t a, spx_uint32_t b)
return a;
}

static spx_uint32_t _muldiv(spx_uint32_t a, spx_uint32_t b, spx_uint32_t c)
{
spx_uint32_t q = 0, r = 0;
spx_uint32_t qn = b / c;
spx_uint32_t rn = b % c;

while(a)
{
if (a & 1)
{
q += qn;
r += rn;
if (r >= c)
{
q++;
r -= c;
}
}
a >>= 1;
qn <<= 1;
rn <<= 1;
if (rn >= c)
{
qn++;
rn -= c;
}
}
return q;
}

EXPORT int speex_resampler_set_rate_frac(SpeexResamplerState *st, spx_uint32_t ratio_num, spx_uint32_t ratio_den, spx_uint32_t in_rate, spx_uint32_t out_rate)
{
spx_uint32_t fact;
Expand All @@ -1133,7 +1135,8 @@ EXPORT int speex_resampler_set_rate_frac(SpeexResamplerState *st, spx_uint32_t r
{
for (i=0;i<st->nb_channels;i++)
{
st->samp_frac_num[i]= _muldiv(st->samp_frac_num[i],st->den_rate,old_den);
if (_muldiv(&st->samp_frac_num[i],st->samp_frac_num[i],st->den_rate,old_den) != RESAMPLER_ERR_SUCCESS)
return RESAMPLER_ERR_OVERFLOW;
/* Safety net */
if (st->samp_frac_num[i] >= st->den_rate)
st->samp_frac_num[i] = st->den_rate-1;
Expand Down