Segmentation fault on invalid .wav file [Patch Included]

Phobius

29-01-2009 05:09:46

I've found and fixed a segfault in OgreAL where if the wav file has an invalid data chunk, the library segfaults by trying to free non-existent buffers. it occurs in two places, the first is Line 127 in OgreALWavSound, where if on any of the header reads we error out, we end up in the catch block and try to erase buffers where mBuffers == NULL due to the Sound() constructor invoked. If we fix that up, the subclass, which was fully instantiated segfaults again in ~Sound() trying to do the same thing. I've fixed this, patch is attached.

Index: OgreALOggSound.cpp
===================================================================
--- OgreALOggSound.cpp (revision 115)
+++ OgreALOggSound.cpp (working copy)
@@ -110,6 +110,7 @@
mLengthInSeconds = ov_time_total(&mOggStream, -1);

generateBuffers();
+ mBuffersLoaded = loadBuffers();
}
catch(Ogre::Exception e)
{
Index: OgreALWavSound.cpp
===================================================================
--- OgreALWavSound.cpp (revision 115)
+++ OgreALWavSound.cpp (working copy)
@@ -118,20 +118,27 @@
mLengthInSeconds = (float)mDataSize / ((float)mBufferSize * 4);

generateBuffers();
+ mBuffersLoaded = loadBuffers();
}
catch(Ogre::Exception e)
- {
- for(int i = 0; i < mNumBuffers; i++)
+ {
+ // If we have gone so far as to load the buffers, unload them.
+ if(mBuffers != NULL) {
+ for(int i = 0; i < mNumBuffers; i++)
{
- if (mBuffers[i] && alIsBuffer(mBuffers[i]) == AL_TRUE)
- {
- alDeleteBuffers(1, &mBuffers[i]);
- CheckError(alGetError(), "Failed to delete Buffer");
- }
+ if (mBuffers[i] && alIsBuffer(mBuffers[i]) == AL_TRUE)
+ {
+ alDeleteBuffers(1, &mBuffers[i]);
+ CheckError(alGetError(), "Failed to delete Buffer");
+ }
}
+ }

- throw (e);
- }
+ // Prevent the ~Sound() destructor from double-freeing.
+ mBuffers = NULL;
+ // Propagate.
+ throw (e);
+ }
}

WavSound::~WavSound()
Index: OgreALSound.cpp
===================================================================
--- OgreALSound.cpp (revision 115)
+++ OgreALSound.cpp (working copy)
@@ -190,9 +190,12 @@

try
{
- alDeleteBuffers(mNumBuffers, mBuffers);
- CheckError(alGetError(), "Failed to delete Buffers, must still be in use.");
- SoundManager::getSingleton()._removeBufferRef(mFileName);
+ // If we have buffers to deallocate, do so.
+ if(mBuffers != NULL) {
+ alDeleteBuffers(mNumBuffers, mBuffers);
+ CheckError(alGetError(), "Failed to delete Buffers, must still be in use.");
+ SoundManager::getSingleton()._removeBufferRef(mFileName);
+ }
}
catch(...)
{
Index: OgreALSoundManager.cpp
===================================================================
--- OgreALSoundManager.cpp (revision 115)
+++ OgreALSoundManager.cpp (working copy)
@@ -442,15 +442,16 @@
** string we know that we've found the double NULL that terminates the
** list and we can stop there.
*/
- while(*deviceList != NULL)
+ while(*deviceList != '\0')
{
try
{
ALCdevice *device = alcOpenDevice(deviceList);
- CheckError(alcGetError(device), "Unable to open device");

if(device)
{
+ CheckError(alcGetError(device), "Unable to open device");
+
// Device seems to be valid
ALCcontext *context = alcCreateContext(device, NULL);
CheckError(alcGetError(device), "Unable to create context");
@@ -466,6 +467,10 @@
CheckError(alcGetError(device), "Unable to destroy context");
}
alcCloseDevice(device);
+ }else{
+ // There is a chance that because the device could not be
+ // opened, the error flag was set, clear it.
+ alcGetError(device);
}
}
catch(...)
@@ -683,7 +688,7 @@
CheckError(alcGetError(NULL), "Failed to retrieve version info");
alcGetIntegerv(NULL, ALC_MINOR_VERSION, sizeof(mMinorVersion), &mMinorVersion);
CheckError(alcGetError(NULL), "Failed to retrieve version info");
-
+
Ogre::LogManager::getSingleton().logMessage("OpenAL Version: " +
Ogre::StringConverter::toString(mMajorVersion) + "." +
Ogre::StringConverter::toString(mMinorVersion));


This thing is a conglomerate patch that also fixed the Ogg vorbis issue I had earlier, and adds compatibility to the new OpenAL-Soft. Any chance I could become a contributor? I don't want to keep posting patches up and I feel i'll be fixing this thing more in the future.

stickymango

30-01-2009 08:39:44

Good work Phobius,

Can I ask why your staying away from OgreOggSound? is there something missing from the library that you require? :?

The only reason I ask is that there isn't anyone actively maintaining this library at present so I think patches/self bug-fixing is the only way to progress OgreAL, I already tried this and found it easier to re-write.

My aim is to provide a free, open-source sound library to the Ogre community, and I'd welcome the help and maintainence from people to ensure OgreOggSound is as robust and useful as it can be. If your interested pop over to the forums or pm me.

Anyway, good work on fixing another bug! :)

Phobius

30-01-2009 10:22:47

The only reason why I am not using OgreOggSound is because linux is not supported. At least not from what I can see. OgreAL is flawed in a number of ways, but it seems to be working so far; it's mostly design flaws which lead to programmatic flaws. If OgreOggSound supported linux i'd jump ship, for sure. I don't have time to port it though, that would take learning your codebase and about two days of conversion. I'm not sure which is worse, using a non-maintained audio playback manager or not porting a codebase I don't understand. I'd gladly fix this bastard up as I plod along with my own project if the admin would let me. I've fixed about four errors now and started to get a solid feel for the code. Meh, I don't know. Linux is where the action is for me, even though i'm porting to windows, the linux support is mandatory.

stickymango

30-01-2009 10:33:13

Ah yes, linux support :(

That's the one big problem with my lib, I don't have access to a linux box and don't know my way around it well enough to attempt a port yet, there was a guy I was working with that had started a port but has since left so things have hit a brick wall.

I'll have to find time to have a stab at it.

Have you tried emailing caseyb?

Fish

31-01-2009 19:58:22

MAC doesn't seem to be supported either.

Phobius

01-02-2009 05:35:06

Yep, no Mac either. I have no issues with windows as a gaming platform, but I am a big supporter of the whole "big three" development when it comes to PC Games. Windows gives you the largest slice of the demographic pie, but linux and mac, although smaller in number, yield greater percentages of users; probably due to the lack of games on these platforms.