Guard against 64-bit overflow in ComputePitch

[Roland Shum (ShumWengSang)]

Guard against 64-bit overflow in ComputePitch

Revised per review feedback: adopts the suggested input bound, which lets the change shrink to the slice computation only.

The defect

ComputePitch computes row and slice pitch in 64-bit arithmetic, but the validation of the result is compiled out on 64-bit targets:

#if defined(_M_IX86) || defined(_M_ARM) || defined(_M_HYBRID_X86_ARM64)
    static_assert(sizeof(size_t) == 4, "Not a 32-bit platform!");
    if (pitch > UINT32_MAX || slice > UINT32_MAX)
    {
        rowPitch = slicePitch = 0;
        return HRESULT_E_ARITHMETIC_OVERFLOW;
    }
#else
    static_assert(sizeof(size_t) == 8, "Not a 64-bit platform!");   // compile-time only
#endif

On 32-bit that check is load-bearing and correct. On 64-bit nothing remains but a static_assert, so slice = pitch * <height-derived multiplier> can wrap modulo 2^64 and the function returns S_OK with a slice pitch smaller than the row pitch.

That breaks the identity the rest of the library relies on:

slicePitch == rowPitch * ComputeScanlines(fmt, height)

Callers use the slice pitch to size allocations and to bounds-check input, while the copy loops are driven by the row pitch and the scanline count. When the two disagree, the size check no longer describes the copy that follows (DirectXTexImage.cpp:66, :370; DirectXTexDDS.cpp:1555, :1654).

Concrete example

No special flags, both dimensions at exactly UINT32_MAX:

BC7_UNORM, width = 4294967295, height = 4294967295, CP_FLAGS_NONE
  ->  S_OK,  rowPitch = 17179869184,  slicePitch = 0

nbw = (0xFFFFFFFF + 3) / 4 = 2^30, pitch = 2^30 * 16 = 2^34, slice = 2^34 * 2^30 = 2^64 → wraps to 0. A 16 GB row pitch reported with a zero slice pitch.

This matters because dimensions are not always bounded by the caller: DecodeDDSHeader deliberately skips the 16384 cap under DDS_FLAGS_ALLOW_LARGE_FILES (DirectXTexDDS.cpp:649-665), and texconv, texassemble and texdiag all set that flag unconditionally for .dds input (texconv.cpp:2078, texassemble.cpp:1415/1442/1465, texdiag.cpp:550).

The change

1. Bound the inputs (as suggested in review):

if (width > UINT32_MAX || height > UINT32_MAX)
    return E_INVALIDARG;

No image format expresses a dimension beyond 32 bits. With this in place every row-pitch computation is provably safe — max bpp is 128, so the largest pitch (PAGE4K) stays under 2^37 — and the incidental additions (+ 3, + 32767, height + ((height + 1) >> 1)) can no longer wrap either.

2. Check the multiplies. Bounding the inputs is not sufficient on its own: with width, height <= UINT32_MAX the row pitch can still reach ~2^36 and the multiplier ~2^33, so slice = pitch * multiplier reaches ~2^69 and wraps. Every pitch and slice multiply therefore goes through a checked helper, and the flag is tested once alongside the existing 32-bit check:

#if defined(__GNUC__) || defined(__clang__)
    if (__builtin_mul_overflow(a, b, &result)) { overflow = true; return 0; }
#elif defined(_MSC_VER) && (defined(_M_ARM64) || defined(_M_ARM64EC))
    const uint64_t high = __umulh(a, b);
    const uint64_t result = a * b;
    if (high != 0) { overflow = true; return 0; }
#elif defined(_MSC_VER) && defined(_M_X64)
    uint64_t high = 0;
    const uint64_t result = _umul128(a, b, &high);
    if (high != 0) { overflow = true; return 0; }
#else
    const uint64_t result = a * b;
    if ((a != 0) && ((result / a) != b)) { overflow = true; return 0; }
#endif

Confirmed by preprocessor output that x64 selects _umul128, ARM64/ARM64EC selects __umulh, and x86 falls to the portable division check (no 64-bit widening intrinsic there). intrin.h arrives via DirectXMath, so no new include is needed. <intsafe.h> was avoided since this also builds for WSL/Linux and macOS.

This detects overflow; it does not impose a magnitude cap. Slice pitches above UINT32_MAX remain valid on 64-bit, so a 16384 x 16384 R32G32B32A32_FLOAT surface — the D3D12 maximum 2D dimension, requiring no special flags, whose slice pitch is exactly 2^32 — continues to work.

Validation

Compiles clean with no warnings: MSVC /W4 x64, MSVC /W4 x86, and clang-cl -Wall -Wextra. Full x64 Release library builds via CMake.

ComputePitch was swept over 107,712 combinations — 17 formats covering every branch of the switch, 24 widths x 24 heights (powers of two, off-by-ones, 16384/16385, 0x80000000, 0xFFFFFFFF), and all 11 CP_FLAGS — with results compared against an unpatched build. Wrap detection uses the identity above (CP_FLAGS_BAD_DXTN_TAILS excluded, since it intentionally uses height >> 2 where ComputeScanlines uses max(1, (height + 3) / 4)).

build	S_OK	rejected	silent wraps
unpatched main	99,792	0	1,413
input bound only	99,792	0	1,413
this PR	98,230	1,562	0

The middle row is why the slice checks are retained: bounding the inputs alone changes nothing measurable, because every dimension involved is already <= UINT32_MAX — the overflow is in the product, not the operands.

Behavioural diff against unpatched, same 107,712 vectors:

	count
differing	1,562
...S_OK -> HRESULT_E_ARITHMETIC_OVERFLOW	1,562 (100%)
...any other change	0
still returning S_OK	98,230
...byte-identical to unpatched	98,230 (100%)

Every previously-succeeding non-overflowing case returns bit-identical values. Spot checks:

case	result
width > UINT32_MAX	E_INVALIDARG (new bound)
BC7_UNORM 0xFFFFFFFF x 0xFFFFFFFF	HRESULT_E_ARITHMETIC_OVERFLOW
R32G32B32A32_FLOAT 16384x16384	S_OK, 262144 / 4294967296 — unchanged
NV12 65536x65536	S_OK, 65536 / 6442450944 — unchanged
R8G8B8A8_UNORM 4096x4096	S_OK, 16384 / 67108864 — unchanged

Notes

• CP_FLAGS_LIMIT_4GB does not cover this. DetermineImageArray (DirectXTexImage.cpp:127) tests totalPixelSize, which is the sum of already-wrapped slice pitches — it runs downstream of the wrap and cannot observe it.
• The accumulator in DetermineImageArray is already defended. SetupImageArray (:197-201) walks pixels += slicePitch and fails once the running pointer passes pEndBits, catching a wrapped total incrementally. No change made there.
• A slicePitch == rowPitch * ComputeScanlines(...) assertion in CopyImage was considered and rejected — the identity does not hold under CP_FLAGS_BAD_DXTN_TAILS, so it would fire on legitimate legacy DXTn content.

[Chuck Walbourn (walbourn)]

The function uses 64-bit integer math already to deal with overflow detection. If there are specific input values that overflow, please provide some examples so we can verify any fix/change here.

I think the majority of this PR is unnecessary. The one change I do believe may be needed is an initial bounds-check on the size width/height values coming in for 64-bit builds (for 32-bit builds is already going to be bounded by UINT32_MAX). In practice, most of the calling code has already done the bounds check but it's reasonable to add it here since ComputePitch is a public-facing API.

IOW, the only change I think is needed here is:

_Use_decl_annotations_
HRESULT DirectX::ComputePitch(DXGI_FORMAT fmt, size_t width, size_t height,
    size_t& rowPitch, size_t& slicePitch, CP_FLAGS flags) noexcept
{
    uint64_t pitch = 0;
    uint64_t slice = 0;

    if (width > UINT32_MAX || height > UINT32_MAX)
        return E_INVALIDARG;

    switch (static_cast<int>(fmt))
    {
    case DXGI_FORMAT_UNKNOWN:
        return E_INVALIDARG;

[Chuck Walbourn (walbourn)]

Most of this is not needed. You are free to submit a revision that only adds the initial bounds check to make sure the values aren't exceeding 32-bit to begin with.

[Roland Shum (ShumWengSang)]

The overflow check in ComputePitch only exists on 32-bit builds:

#if defined(_M_IX86) || defined(_M_ARM) || defined(_M_HYBRID_X86_ARM64)
    if (pitch > UINT32_MAX || slice > UINT32_MAX)
        return HRESULT_E_ARITHMETIC_OVERFLOW;
#else
    static_assert(sizeof(size_t) == 8, "Not a 64-bit platform!");
#endif

On x64 nothing inspects the result at runtime. slice = pitch * scanlines silently truncates to 64 bits and the function returns S_OK with a slice pitch smaller than the row pitch. Callers size allocations from the slice pitch but copy using the row pitch, so the two stop agreeing.

Measured on x64, CP_FLAGS_NONE:

format	width x height	row pitch	correct slice pitch	main returns	this PR
BC7_UNORM	4294967295 x 4294967295	17,179,869,184	18,446,744,073,709,551,616	0	ARITHMETIC_OVERFLOW
R8G8B8A8_UNORM	2147483648 x 2147483648	8,589,934,592	18,446,744,073,709,551,616	0	ARITHMETIC_OVERFLOW
R32G32B32A32_FLOAT	4294967295 x 4294967295	68,719,476,720	295,147,905,041,913,872,400	18,446,743,936,270,598,160	ARITHMETIC_OVERFLOW
NV12	4294967294 x 4294967294	4,294,967,294	27,670,116,084,794,523,654	9,223,372,011,084,972,038	ARITHMETIC_OVERFLOW
R32G32B32A32_FLOAT	16384 x 16384	262,144	4,294,967,296	4,294,967,296	unchanged
R8G8B8A8_UNORM	4096 x 4096	16,384	67,108,864	67,108,864	unchanged

I've added your bounds check and it's a good one — it makes every row-pitch computation provably safe, so I dropped 16 of the 34 changed lines. But it doesn't catch any of the rows above: both dimensions are already <= UINT32_MAX, so it never fires. The overflow is in the product, not the operands.

The last two rows are why I didn't simply enable the existing > UINT32_MAX check on x64. A 16384 x 16384 R32G32B32A32_FLOAT surface is legal at the D3D12 dimension limit and its slice pitch is exactly 2^32 — a magnitude cap would stop it loading. So the fix detects the wrap instead of capping the value.

Swept 107,712 format/size/flag combinations against an unpatched build: all 98,230 previously-succeeding cases return byte-identical values, and the only change is 1,562 cases that used to truncate now returning the overflow HRESULT. Builds clean on MSVC /W4 x64 and x86, and clang-cl -Wall -Wextra.

[Roland Shum (ShumWengSang)]

Both applied in 550e28f.

Intrinsics — added, and confirmed via preprocessor output which branch each target actually takes:

target	branch selected
x64	_umul128
ARM64 / ARM64EC	__umulh
x86	portable division check
GCC / Clang	__builtin_mul_overflow

You were right that intrin.h is already available — no new include needed. Builds clean with no warnings on MSVC /W4 x64, /W4 x86, /W4 ARM64, and clang-cl -Wall -Wextra.

One deviation from your snippet: I left _M_HYBRID_X86_ARM64 off the __umulh branch. That target is 32-bit (it's grouped with _M_IX86/_M_ARM under the existing static_assert(sizeof(size_t) == 4)), so it falls to the portable path with the rest of the 32-bit targets. Happy to add it if __umulh is in fact available there — I don't have a CHPE toolchain to verify, and I'd rather not assert it untested.

Helper on both pitch and slice — agreed, and done. My reasoning for restricting it to slice was that the input bound makes the pitch computations provably safe (max bpp 128, so pitch stays under 2^37), but you're right that this is a distinction the next reader has to re-derive, and it quietly breaks if the bound is ever relaxed. Uniform is better.

No behaviour change from either edit — the sweep is identical to the previous revision, and against unpatched main: 98,230 of 98,230 still-succeeding cases byte-identical, all 1,562 differences S_OK -> HRESULT_E_ARITHMETIC_OVERFLOW, zero silent wraps remaining.

The 8 red checks are unrelated to this change: 7 are an MSVC internal compiler error (C1001) building openjph for x86 under vcpkg, and 1 is a DNS failure fetching the openjph tarball. The same 7 appear on #515. Non-vcpkg x86 and all x64 configurations pass.

[Chuck Walbourn (walbourn)]

Let's just remove the early-out bound check and we can rely on proper overflow detection.

[Chuck Walbourn (walbourn)]

It might also make it easier to test the failure points below without this guard :)

[Chuck Walbourn (walbourn)]

Since this is now a function call, we can remove the extra casting here to simplify the code.