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

Potential division by zero in SoundStream::streamData() #529

Closed
Gaerzi opened this Issue Feb 3, 2014 · 5 comments

Comments

Projects
None yet
3 participants
@Gaerzi

Gaerzi commented Feb 3, 2014

While working on integrating Game Music Emu into a game-centric resource viewer/player so that it'd play chiptunes (in addition to streamed audio, tracker modules via libmodplug and sfMod, and MIDI via FluidSynth), I got crashes while attempting to play back NSF and HES chiptunes. The crashes happened within SFML itself. Now they're probably because I did something dumb somewhere upstream of SFML, but nevertheless, the crash can be avoided on SFML's side.

The problem comes from the fact there is a division by a value done without checking that the value is actually not null (if bits is less than 8, it'll become null after integer division). I worked around that with a ?:1 but a MIN(,1) would also work. An assert or error logger check could be placed there maybe.

            // Retrieve its size and add it to the samples count
            if (m_endBuffers[bufferNum])
            {
                // This was the last buffer: reset the sample count
                m_samplesProcessed = 0;
                m_endBuffers[bufferNum] = false;
            }
            else
            {
                ALint size, bits;
                alCheck(alGetBufferi(buffer, AL_SIZE, &size));
                alCheck(alGetBufferi(buffer, AL_BITS, &bits));
                m_samplesProcessed += size / ((bits / 8) ? (bits / 8) : 1);
            }

Now on my side I get to try to understand why OpenAL reports less than 8 bits. Samples are supposed to be 16 bits.

@LaurentGomila LaurentGomila self-assigned this Feb 3, 2014

@Bromeon Bromeon assigned Bromeon and unassigned LaurentGomila Feb 9, 2014

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Feb 23, 2014

Member

It would definitely be good to know why OpenAL returns a value of zero. What I could find so far is a forum thread of the Slick2D Java Game Library. They have exactly the same problem. Line 38 and surroundings of their AudioImpl.java looks as follows:

int bytes = AL10.alGetBufferi(buffer, AL10.AL_SIZE);
int bits = AL10.alGetBufferi(buffer, AL10.AL_BITS);
int channels = AL10.alGetBufferi(buffer, AL10.AL_CHANNELS);
int freq = AL10.alGetBufferi(buffer, AL10.AL_FREQUENCY);

int samples = bytes / (bits / 8);
length = (samples / (float) freq) / channels;

It seems like a format problem in the OGG file.

However, I don't think a good solution is to just ignore this error with operator?:. Did you get an error message on the console? Do you happen to have a reproducing example? Does the problem occur only with specific (probably corrupt) OGG files?

Member

Bromeon commented Feb 23, 2014

It would definitely be good to know why OpenAL returns a value of zero. What I could find so far is a forum thread of the Slick2D Java Game Library. They have exactly the same problem. Line 38 and surroundings of their AudioImpl.java looks as follows:

int bytes = AL10.alGetBufferi(buffer, AL10.AL_SIZE);
int bits = AL10.alGetBufferi(buffer, AL10.AL_BITS);
int channels = AL10.alGetBufferi(buffer, AL10.AL_CHANNELS);
int freq = AL10.alGetBufferi(buffer, AL10.AL_FREQUENCY);

int samples = bytes / (bits / 8);
length = (samples / (float) freq) / channels;

It seems like a format problem in the OGG file.

However, I don't think a good solution is to just ignore this error with operator?:. Did you get an error message on the console? Do you happen to have a reproducing example? Does the problem occur only with specific (probably corrupt) OGG files?

@Gaerzi

This comment has been minimized.

Show comment
Hide comment
@Gaerzi

Gaerzi Feb 23, 2014

As I said, I was using Game Music Emu so the problem happened while attempting to play instruction streams for an emulated chipset, not with OGG files. (Specifically, it happened with NSF files.) I found out the source of the problem in my code as it came from a confusion between GME channels (read: instruments) and output channels (mono/stereo/etc.). Hardcoding 2-channel stereo in the call to sf::SoundStream::initialize() instead of wrongly using the instrument count made the problem disappear. NSF files have five instruments, so in the bogus code it initialized the soundstream with an odd number of channels, whereas most other formats supported by GME have an even number of instruments, which hid the issue until I grabbed some NSFs for testing.

So, it was basically a format problem which caused the problem to happen. I'd suggest using an assert or error logger message to warn that the stream was probably incorrectly initialized if bits is less than 8.

Gaerzi commented Feb 23, 2014

As I said, I was using Game Music Emu so the problem happened while attempting to play instruction streams for an emulated chipset, not with OGG files. (Specifically, it happened with NSF files.) I found out the source of the problem in my code as it came from a confusion between GME channels (read: instruments) and output channels (mono/stereo/etc.). Hardcoding 2-channel stereo in the call to sf::SoundStream::initialize() instead of wrongly using the instrument count made the problem disappear. NSF files have five instruments, so in the bogus code it initialized the soundstream with an odd number of channels, whereas most other formats supported by GME have an even number of instruments, which hid the issue until I grabbed some NSFs for testing.

So, it was basically a format problem which caused the problem to happen. I'd suggest using an assert or error logger message to warn that the stream was probably incorrectly initialized if bits is less than 8.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Feb 23, 2014

Member

I'd suggest using an assert or error logger message to warn that the stream was probably incorrectly initialized if bits is less than 8.

Generally I think that's a good idea. Do you (or somebody else) happen to know whether such errors can be caught in a "clean", documented way, maybe earlier in the code? The bits being 0 sounds like a specific side-effect on which we would have to rely...

Member

Bromeon commented Feb 23, 2014

I'd suggest using an assert or error logger message to warn that the stream was probably incorrectly initialized if bits is less than 8.

Generally I think that's a good idea. Do you (or somebody else) happen to know whether such errors can be caught in a "clean", documented way, maybe earlier in the code? The bits being 0 sounds like a specific side-effect on which we would have to rely...

@Gaerzi

This comment has been minimized.

Show comment
Hide comment
@Gaerzi

Gaerzi Feb 23, 2014

Even in debug mode, I didn't get any error message from OpenAL or SFML before that point, no.

Gaerzi commented Feb 23, 2014

Even in debug mode, I didn't get any error message from OpenAL or SFML before that point, no.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Feb 23, 2014

Member

Ok... so I'll implement this specific check (bits != 0), it can still be refined in the future. The whole line

m_samplesProcessed += size / (bits / 8);

would then not be executed. I don't think it makes sense to process any further in case of error, thus I would exit both nested loops in SoundStream::streamData() and only perform the cleanup code at the end of the function.

Member

Bromeon commented Feb 23, 2014

Ok... so I'll implement this specific check (bits != 0), it can still be refined in the future. The whole line

m_samplesProcessed += size / (bits / 8);

would then not be executed. I don't think it makes sense to process any further in case of error, thus I would exit both nested loops in SoundStream::streamData() and only perform the cleanup code at the end of the function.

@Bromeon Bromeon closed this in f9233e7 Mar 25, 2014

@Bromeon Bromeon added the resolved label Mar 25, 2014

MarioLiebisch added a commit to MarioLiebisch/SFML that referenced this issue Apr 15, 2014

jcowgill added a commit to jcowgill/SFML that referenced this issue Sep 22, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment