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.

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

Handle memory allocation failures, and reduce overflow #4

Closed
wants to merge 2 commits into from

Conversation

jyavenard
Copy link
Contributor

with comments addressed.

@jyavenard
Copy link
Contributor Author

r? @tmatth

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.

@tmatth
Copy link
Member

tmatth commented May 10, 2016

I understand that slow review and resubmitting patches over details is tedious. Please keep in mind that this is just a github mirror of speexdsp. Normally, development takes place on the speex-dev mailing list (hence this PR flew under my radar). As is too often the case, review/dev/maintenance is done on a best effort basis by volunteers.
In any case, thanks for the contribution, will merge today. Please feel free to ping me or jmspeex on #speex if you'd like to talk further.

@tmatth
Copy link
Member

tmatth commented May 10, 2016

Applied in 8c2981f and 335a9a1

@tmatth tmatth closed this May 10, 2016
@jyavenard
Copy link
Contributor Author

I couldn't find on the speex website a description about how to best handle a change request.
Nor could I find details on what the coding style is, so I took a shot. So I have to admit that I got frustrated as the target to reach appeared to be a moving one.
The main reason I didn't want to assume that result wasn't null, was that it allowed to use the _muldiv to only check for overflow, without actually performing the operation. There's a few other places in the code doing a = a*b/c, all of those appeared fine to me, but you can't never be sure about it.

Maybe, just to be safe and avoid all the issues alltogether, would be to have a quick check at the beginning of the resampling operation to check that the number of frames and the number of channels are reasonable (and in particular that frames * channels * sizeof(float) will not overflow) and abort if not. I understand that speex had the assumptions that the incoming values were reasonable. Unfortunately, the way firefox used it broke those assumptions. FWIW, speex can't be called with those crazy user input anymore.

@tmatth
Copy link
Member

tmatth commented May 12, 2016

Maybe, just to be safe and avoid all the issues alltogether, would be to have a quick check at the
beginning of the resampling operation to check that the number of frames and the number of
channels are reasonable (and in particular that frames * channels * sizeof(float) will not overflow)
and abort if not.

Sounds reasonable to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants