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.
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.
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.
