search

xiph / speexdsp

Speex audio processing library - THIS IS A MIRROR, DEVELOPMENT HAPPENS AT https://gitlab.xiph.org/xiph/speexdsp
https://speex.org
Other
469 stars 190 forks source link

Handle memory allocation failures, and reduce overflow #4

Closed jyavenard closed 8 years ago

jyavenard commented 8 years ago

with comments addressed.

jyavenard commented 8 years ago

r? @tmatth

tmatth commented 8 years ago

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 commented 8 years ago

Applied in 8c2981fa4527330708fccda4de3490a60168d568 and 335a9a16b8a90fee92002a7256b90bb6d8498622

jyavenard commented 8 years ago

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 commented 8 years ago

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.