DDS files with a DDS_PIXELFORMAT size of 24 about directxtex HOT 12 CLOSED

The number of ways people have messed up DDS details over the years is amazing :)

Looking at the struct definition, the only valid value I see is 32. I know there are variants with 0, but I don't understand how they got to 24...

typedef struct _DDPIXELFORMAT {
  DWORD dwSize;
  DWORD dwFlags;
  DWORD dwFourCC;
  union {
    DWORD dwRGBBitCount;
    DWORD dwYUVBitCount;
    DWORD dwZBufferBitDepth;
    DWORD dwAlphaBitDepth;
  };
  union {
    DWORD dwRBitMask;
    DWORD dwYBitMask;
  };
  union {
    DWORD dwGBitMask;
    DWORD dwUBitMask;
  };
  union {
    DWORD dwBBitMask;
    DWORD dwVBitMask;
  };
  union {
    DWORD dwRGBAlphaBitMask;
    DWORD dwYUVAlphaBitMask;
    DWORD dwRGBZBitMask;
    DWORD dwYUVZBitMask;
  };
} DDPIXELFORMAT, *LPDDPIXELFORMAT;

from directxtex.

I don't know where the hell 24 came from.

I do not know how they got that value either. The only thing I can think of is that they may have assumed that only the unused fields are included in the size.

I already have a fix in my fork, should I submit it as a PR?

from directxtex.

Feel free to submit a PR. Accepting 0, 24, or 32 as 'valid'

from directxtex.

I wonder how many other variants there are... I don't want to skip validating the value entirely though for security reasons.

from directxtex.

Something like the following including an extra check in the "DX10"extension header case since these weird variants only make sense for DX9 legacy files.

        if (pHeader->ddspf.size != 0 /* Known variant */
            && pHeader->ddspf.size != 24 /* Known variant */
            && pHeader->ddspf.size != sizeof(DDS_PIXELFORMAT))
        {
            return E_FAIL;
        }

        metadata.mipLevels = pHeader->mipMapCount;
        if (metadata.mipLevels == 0)
            metadata.mipLevels = 1;

        // Check for DX10 extension
        if ((pHeader->ddspf.flags & DDS_FOURCC)
            && (MAKEFOURCC('D', 'X', '1', '0') == pHeader->ddspf.fourCC))
        {
            if (pHeader->ddspf.size != sizeof(DDS_PIXELFORMAT))
            {
                return E_FAIL;
            }

            // Buffer must be big enough for both headers and magic value
            if (size < (sizeof(DDS_HEADER) + sizeof(uint32_t) + sizeof(DDS_HEADER_DXT10)))
            {
                return E_FAIL;
            }

from directxtex.

I wonder how many other variants there are... I don't want to skip validating the value entirely though for security reasons.

The most common error I have seen is DDS files that specify one too many mipmaps in the header.
For example, a 1024x1024 pixel file that specifies 12 mipmaps instead of 11.

DirectXTex correctly rejects these files as invalid, but it would be nice to have an option to allow those files to be loaded.
Currently, I have to instruct users to re-save those files using a different program that does not validate the mipmap count when loading the file.
I had looked at adding that to my fork, but I could never determine the best way to implement it.

from directxtex.

Is it always an off-by-one thing?

from directxtex.

Is it always an off-by-one thing?

Yes.

from directxtex.

Can you attach an example or two?

from directxtex.

The following file is from this issue in the pdn-ddsfiletype-plus repository. It is a 256x256 pixel image that specifies 10 mipmaps instead of 9.
The OP said that they found a few other files with the same off-by-one issue.

beam_lo.zip

Here is another 256x256 pixel file with 10 mipmaps that I created: mip-off-by-1.zip

I also think I may be overestimating the prevalence of this bug, as I often forget that dwMipMapCount includes the main image.

Should this discussion be split into its own issue?

from directxtex.

Thanks for the details. I think my answer to this is:

1. I'm going to add a DDS_FLAGS_PERMISSIVE flag to the code.
2. When this flag is provided, it will accept a mipmap level that is "correct+1"
3. I'm going to move your "DDPF.dwSize ==0" and "DDPF.dwSize == 24" cases to require the DDS_FLAGS_PERMISSIVE option so that without it you get the 'original 'behavior of failing on files that are malformed.
4. I can add further variant support through guarding it with this flag.

Sound good to you?

from directxtex.

Yes, that is fine.

from directxtex.