texconv: Fix PFM reader to avoid overread of buffer (#413)

* texconv: Fix PFM reader to avoid overread of buffer

* More code review

* Max memory usage

* YAML fix

DirectXTex/DirectXTexD3D12.cpp

@@ -121,14 +121,19 @@ namespace
         _In_ ID3D12CommandQueue* pCommandQ,
         _In_ ID3D12Resource* pSource,
         const D3D12_RESOURCE_DESC& desc,
-        ComPtr<ID3D12Resource>& pStaging,
+        _COM_Outptr_ ID3D12Resource** pStaging,
         std::unique_ptr<uint8_t[]>& layoutBuff,
         UINT& numberOfPlanes,
         UINT& numberOfResources,
         D3D12_RESOURCE_STATES beforeState,
         D3D12_RESOURCE_STATES afterState) noexcept
     {
-        if (!pCommandQ || !pSource)
+        if (pStaging)
+        {
+            *pStaging = nullptr;
+        }
+
+        if (!pCommandQ || !pSource || !pStaging)
             return E_INVALIDARG;
 
         numberOfPlanes = D3D12GetFormatPlaneCount(device, desc.Format);
@@ -170,7 +175,8 @@ namespace
         if (SUCCEEDED(hr) && sourceHeapProperties.Type == D3D12_HEAP_TYPE_READBACK)
         {
             // Handle case where the source is already a staging texture we can use directly
-            pStaging = pSource;
+            *pStaging = pSource;
+            pSource->AddRef();
             return S_OK;
         }
 
@@ -267,11 +273,11 @@ namespace
             &bufferDesc,
             D3D12_RESOURCE_STATE_COPY_DEST,
             nullptr,
-            IID_GRAPHICS_PPV_ARGS(pStaging.GetAddressOf()));
+            IID_GRAPHICS_PPV_ARGS(pStaging));
         if (FAILED(hr))
             return hr;
 
-        assert(pStaging);
+        assert(*pStaging);
 
         // Transition the resource if necessary
         TransitionResource(commandList.Get(), pSource, beforeState, D3D12_RESOURCE_STATE_COPY_SOURCE);
@@ -279,7 +285,7 @@ namespace
         // Get the copy target location
         for (UINT j = 0; j < numberOfResources; ++j)
         {
-            const CD3DX12_TEXTURE_COPY_LOCATION copyDest(pStaging.Get(), pLayout[j]);
+            const CD3DX12_TEXTURE_COPY_LOCATION copyDest(*pStaging, pLayout[j]);
             const CD3DX12_TEXTURE_COPY_LOCATION copySrc(copySource.Get(), j);
             commandList->CopyTextureRegion(&copyDest, 0, 0, 0, &copySrc, nullptr);
         }
@@ -734,7 +740,7 @@ HRESULT DirectX::CaptureTexture(
         pCommandQueue,
         pSource,
         desc,
-        pStaging,
+        pStaging.GetAddressOf(),
         layoutBuff,
         numberOfPlanes,
         numberOfResources,

ScreenGrab/ScreenGrab12.cpp

@@ -687,11 +687,16 @@ namespace
         _In_ ID3D12Resource* pSource,
         UINT64 srcPitch,
         const D3D12_RESOURCE_DESC& desc,
-        ComPtr<ID3D12Resource>& pStaging,
+        _COM_Outptr_ ID3D12Resource** pStaging,
         D3D12_RESOURCE_STATES beforeState,
         D3D12_RESOURCE_STATES afterState) noexcept
     {
-        if (!pCommandQ || !pSource)
+        if (pStaging)
+        {
+            *pStaging = nullptr;
+        }
+
+        if (!pCommandQ || !pSource || !pStaging)
             return E_INVALIDARG;
 
         if (desc.Dimension != D3D12_RESOURCE_DIMENSION_TEXTURE2D)
@@ -709,7 +714,8 @@ namespace
         if (SUCCEEDED(hr) && sourceHeapProperties.Type == D3D12_HEAP_TYPE_READBACK)
         {
             // Handle case where the source is already a staging texture we can use directly
-            pStaging = pSource;
+            *pStaging = pSource;
+            pSource->AddRef();
             return S_OK;
         }
 
@@ -801,11 +807,11 @@ namespace
             D3D12_RESOURCE_STATE_COPY_DEST,
             nullptr,
             IID_ID3D12Resource,
-            reinterpret_cast<void**>(pStaging.ReleaseAndGetAddressOf()));
+            reinterpret_cast<void**>(pStaging));
         if (FAILED(hr))
             return hr;
 
-        assert(pStaging);
+        assert(*pStaging);
 
         // Transition the resource if necessary
         TransitionResource(commandList.Get(), pSource, beforeState, D3D12_RESOURCE_STATE_COPY_SOURCE);
@@ -818,7 +824,7 @@ namespace
         bufferFootprint.Footprint.RowPitch = static_cast<UINT>(srcPitch);
         bufferFootprint.Footprint.Format = desc.Format;
 
-        const CD3DX12_TEXTURE_COPY_LOCATION copyDest(pStaging.Get(), bufferFootprint);
+        const CD3DX12_TEXTURE_COPY_LOCATION copyDest(*pStaging, bufferFootprint);
         const CD3DX12_TEXTURE_COPY_LOCATION copySrc(copySource.Get(), 0);
 
         // Copy the texture
@@ -929,7 +935,7 @@ HRESULT DirectX::SaveDDSTextureToFile(
         return HRESULT_E_ARITHMETIC_OVERFLOW;
 
     ComPtr<ID3D12Resource> pStaging;
-    HRESULT hr = CaptureTexture(device.Get(), pCommandQ, pSource, dstRowPitch, desc, pStaging, beforeState, afterState);
+    HRESULT hr = CaptureTexture(device.Get(), pCommandQ, pSource, dstRowPitch, desc, pStaging.GetAddressOf(), beforeState, afterState);
     if (FAILED(hr))
         return hr;
 
@@ -1157,7 +1163,7 @@ HRESULT DirectX::SaveWICTextureToFile(
         return HRESULT_E_ARITHMETIC_OVERFLOW;
 
     ComPtr<ID3D12Resource> pStaging;
-    HRESULT hr = CaptureTexture(device.Get(), pCommandQ, pSource, dstRowPitch, desc, pStaging, beforeState, afterState);
+    HRESULT hr = CaptureTexture(device.Get(), pCommandQ, pSource, dstRowPitch, desc, pStaging.GetAddressOf(), beforeState, afterState);
     if (FAILED(hr))
         return hr;
 

Texconv/PortablePixMap.cpp

@@ -264,7 +264,7 @@ HRESULT __cdecl LoadFromPortablePixMap(
 
                 if (u > INT32_MAX)
                 {
-                    return HRESULT_FROM_WIN32(ERROR_FILE_TOO_LARGE);
+                    return HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED);
                 }
 
                 width = u;
@@ -277,7 +277,13 @@ HRESULT __cdecl LoadFromPortablePixMap(
 
                     if (u > INT32_MAX)
                     {
-                        return HRESULT_FROM_WIN32(ERROR_FILE_TOO_LARGE);
+                        return HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED);
+                    }
+
+                    uint64_t sizeBytes = uint64_t(width) * uint64_t(u);
+                    if (sizeBytes > UINT32_MAX)
+                    {
+                        HRESULT_FROM_WIN32(ERROR_ARITHMETIC_OVERFLOW);
                     }
 
                     if (metadata)
@@ -473,7 +479,7 @@ HRESULT __cdecl LoadFromPortablePixMapHDR(
     size_t len = 0;
     while (pfmSize > 0)
     {
-        len = FindEOL(pData, 256);
+        len = FindEOL(pData, std::min<size_t>(256, pfmSize));
         if (!len)
             return E_FAIL;
 
@@ -498,9 +504,15 @@ HRESULT __cdecl LoadFromPortablePixMapHDR(
     if (sscanf_s(dataStr, "%zu %zu%s", &width, &height, junkStr, 256) != 2)
         return E_FAIL;
 
-    if ((width > INT32_MAX) || (height > UINT32_MAX))
+    if ((width > INT32_MAX) || (height > INT32_MAX))
     {
-        return HRESULT_FROM_WIN32(ERROR_FILE_TOO_LARGE);
+        return HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED);
+    }
+
+    uint64_t sizeBytes = uint64_t(width) * uint64_t(height);
+    if (sizeBytes > UINT32_MAX)
+    {
+        HRESULT_FROM_WIN32(ERROR_ARITHMETIC_OVERFLOW);
     }
 
     pData += len + 1;
@@ -516,7 +528,7 @@ HRESULT __cdecl LoadFromPortablePixMapHDR(
     len = 0;
     while (pfmSize > 0)
     {
-        len = FindEOL(pData, 256);
+        len = FindEOL(pData, std::min<size_t>(256, pfmSize));
         if (!len)
             return E_FAIL;
 
@@ -550,8 +562,8 @@ HRESULT __cdecl LoadFromPortablePixMapHDR(
     if (!pfmSize)
         return E_FAIL;
 
-    const size_t scanline = width * (half16 ? sizeof(uint16_t) : sizeof(float)) * (monochrome ? 1 : 3);
+    const uint64_t scanline = uint64_t(width) * (half16 ? sizeof(uint16_t) : sizeof(float)) * (monochrome ? 1 : 3);
-    if (pfmSize < scanline * height)
+    if (uint64_t(pfmSize) < (scanline * uint64_t(height)))
         return HRESULT_FROM_WIN32(ERROR_HANDLE_EOF);
 
     if (metadata)
@@ -570,7 +582,6 @@ HRESULT __cdecl LoadFromPortablePixMapHDR(
 
     auto img = image.GetImage(0, 0, 0);
 
-
     if (half16)
     {
         auto sptr = reinterpret_cast<const uint16_t*>(pData);

build/DirectXTex-SDL.yml

@@ -32,6 +32,8 @@ pool:
 jobs:
 - job: SDL_BUILD
   displayName: 'Build using required SDL tools'
+  workspace:
+    clean: all
   steps:
   - checkout: self
     clean: true

build/OneFuzzConfig.json

@@ -12,7 +12,10 @@
       "OneFuzzJobs": [
         {
           "ProjectName": "Direct3D",
-          "TargetName": "DirectXTex"
+          "TargetName": "DirectXTex",
+          "TargetOptions": [
+             "-rss_limit_mb=4096"
+          ]
         }
       ],
       "JobDependencies": [