Heap Corruption while loading DDS

[0xC0DEFACE]

Hi.

When loading the attached dds file I get a heap corruption within the OgreDDSCodec.cpp file. The dds file seems to load fine in other programs. If I save it out from other programs it still seems to crash in ogre. The unremarkable except its dimensions are non power of two. I feel this is causing a miscalculation somewhere however I have not been able to spot the issue.

To catch the heap corruption in the act (in VS on windows 10) I had to use the MS application verifier combined with the VS 2013 debugger. Essentially the main loop of the decode function writes past the end of the "output" MemoryDataStream.

I tried grabbing the latest DDSCodec file from the 1.10 branch and the 2.1 branch in case there were any relevant fixes, but there were none that I could see.

If anyone else could have a peek and see if they could repro I would greatly appreciate it.

It should be render system agnostic. In fact im running the code in a offline tool and using Ogre::Image to load the dds.

Edit: It is not Render system agnostic! The code with the issue only runs when there is no render system initialised. Its decompressing the dxt file into ABGR and that is what is causing the error.

Thanks!

[SolarPortal]

I tested your image on the older DX9 and the new 2.1 OpenGL and it seems to be fine

However, i am also experiencing some strange texture problems especially with decode using .tga's.
Check this thread out: http://www.ogre3d.org/forums/viewtopic.php?f=25&t=85561

[0xC0DEFACE]

I tested your image on the older DX9 and the new 2.1 OpenGL and it seems to be fine

Its a heap corruption, so it wont always cause a crash, however if you put a conditional break point here:

Code: Select all

										// advance to next row
										destPtr = static_cast<void*>(
											static_cast<uchar*>(destPtr) + destPitchMinus4);

With the condition

Code: Select all

destPtr > output.mPos + imgData->size

Then it should break when the heap corruption occurs.

[mkultra333]

I had trouble with a particular DDS in a project I'm updating from 1.8 to 1.10. Other DDS files loaded fine. These are DXT compressed types.

I vaguely recall there was some problem with some DDS files in some code long ago, not standard Ogre code, that allowed loading the lower mipmaps of DDS files. It turned out that some weren't compressed properly, or weren't quite following the format, or something like that.

Most of my DDS files are compressed using Nvidia Texture Tools programs, but a few were compressed using an Nvidia Photoshop plug-in. I wonder if the photoshop plugin had bugs.

It might be worth trying to compress the image again using Nvidia Texture Tools and see if it works.

[0xC0DEFACE]

Like I said when I save it out of other programs it still crashes.

I've also now taken a bunch of power of 2 DXT5 images that are loading fine, and using DX texconv.exe have resized them to 140x140 with 7 mips. After that they all also cause a heap corruption. This certainly seems to point towards a bug in ogre loading code.

When anyone is testing just remember that a heap corruption doesn't always cause a crash. This is why they are such insidious little bugs. Silently corrupting memory, causing crashes and errors later in other parts of the program.

This seems like a bug to me. 140x140 DXT5 textures with mips cause a buffer overrun in the DDSCodec.

[0xC0DEFACE]

I made an edit on my initial post. Basically is not Render system agnostic! The code with the issue only runs when there is no render system initialised, which is why I'm getting the issue on an offline tool. Its decompressing the dxt file into ABGR rather than loading it straight into memory as dxt. I probably should have realised that yesterday.

[0xC0DEFACE]

Ok

So with fresh eyes today I have found and fixed the issue, and it was indeed not taking into account non powers of two.

here is the before:

Code: Select all

					// Compressed data
					if (decompressDXT)
					{
						DXTColourBlock col;
						DXTInterpolatedAlphaBlock iAlpha;
						DXTExplicitAlphaBlock eAlpha;
						// 4x4 block of decompressed colour
						ColourValue tempColours[16];
						size_t destBpp = PixelUtil::getNumElemBytes(imgData->format);
						size_t sx = std::min((size_t)width, (size_t)4);
						size_t sy = std::min((size_t)height, (size_t)4);
						size_t destPitchMinus4 = dstPitch - destBpp * sx;
						// slices are done individually
						for(size_t z = 0; z < depth; ++z)
						{
							// 4x4 blocks in x/y
							for (size_t y = 0; y < height; y += 4)
							{
								for (size_t x = 0; x < width; x += 4)
								{

And the after:

Code: Select all

						
					// Compressed data
					if (decompressDXT)
					{
						DXTColourBlock col;
						DXTInterpolatedAlphaBlock iAlpha;
						DXTExplicitAlphaBlock eAlpha;
						// 4x4 block of decompressed colour
						ColourValue tempColours[16];
						size_t destBpp = PixelUtil::getNumElemBytes(imgData->format);
						
						// slices are done individually
						for(size_t z = 0; z < depth; ++z)
						{
							size_t remainingWidth = width;
							size_t remainingHeight = height;

							// 4x4 blocks in x/y
							for (size_t y = 0; y < height; y += 4)
							{
								size_t sy = std::min( ( size_t )remainingHeight, ( size_t )4 );
								remainingHeight -= sy;

								for (size_t x = 0; x < width; x += 4)
								{
									size_t sx = std::min((size_t)remainingWidth, (size_t)4);
									size_t destPitchMinus4 = dstPitch - destBpp * sx;

									remainingWidth -= sx;

Here is the patch:

Code: Select all

diff -r b9276451dea4 deps/Ogre/ogre_src_v1-9/OgreMain/src/OgreDDSCodec.cpp
--- a/deps/Ogre/ogre_src_v1-9/OgreMain/src/OgreDDSCodec.cpp	Fri Apr 08 17:49:22 2016 +0100
+++ b/deps/Ogre/ogre_src_v1-9/OgreMain/src/OgreDDSCodec.cpp	Wed Apr 13 13:14:54 2016 +0100
@@ -787,17 +787,26 @@
 						// 4x4 block of decompressed colour
 						ColourValue tempColours[16];
 						size_t destBpp = PixelUtil::getNumElemBytes(imgData->format);
-						size_t sx = std::min((size_t)width, (size_t)4);
-						size_t sy = std::min((size_t)height, (size_t)4);
-						size_t destPitchMinus4 = dstPitch - destBpp * sx;
+						
 						// slices are done individually
 						for(size_t z = 0; z < depth; ++z)
 						{
+							size_t remainingWidth = width;
+							size_t remainingHeight = height;
+
 							// 4x4 blocks in x/y
 							for (size_t y = 0; y < height; y += 4)
 							{
+								size_t sy = std::min( ( size_t )remainingHeight, ( size_t )4 );
+								remainingHeight -= sy;
+
 								for (size_t x = 0; x < width; x += 4)
 								{
+									size_t sx = std::min((size_t)remainingWidth, (size_t)4);
+									size_t destPitchMinus4 = dstPitch - destBpp * sx;
+
+									remainingWidth -= sx;
+
 									if (sourceFormat == PF_DXT2 || 
 										sourceFormat == PF_DXT3)
 									{

Notice that sx and sy are now updated so that on the last iteration of a non power of two texture they will be < 4, preventing the buffer overrun.

I'll generate a pull request shortly.

[dark_sylinc]

I'll generate a pull request shortly.

Add me as a reviewer of the PR so I see it promptly