[SYCL][Test] Fix out-of-bounds accesses in unit test device-image helpers - #23107
Open
uditagarwal97 wants to merge 1 commit into
Open
[SYCL][Test] Fix out-of-bounds accesses in unit test device-image helpers#23107uditagarwal97 wants to merge 1 commit into
uditagarwal97 wants to merge 1 commit into
Conversation
makeSpecConstant() and friends wrote size headers with std::uninitialized_copy(&Scalar, &Scalar + 8, ...), which copies 8 *objects* (64 bytes) rather than 8 bytes, and KernelBundleStateFiltering indexed the address of a parameter instead of the array it points to. The first one aborted three test binaries during --gtest_list_tests, which lit reports as "failed_to_discover_tests_from_gtest". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The multi-aspect serialization fix needs a regression test covering more than one aspect.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes out-of-bounds and incomplete copies in SYCL unit-test device-image helpers.
Changes:
- Replaces incorrect object-range copies with byte-accurate
memcpy. - Correctly serializes multiple aspects.
- Safely tracks the first shared binary in multi-device tests.
File summaries
| File | Description |
|---|---|
sycl/unittests/SYCL2020/KernelBundleStateFiltering.cpp |
Fixes binary pointer indexing. |
sycl/unittests/helpers/MockDeviceImage.hpp |
Fixes property serialization copies. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
KseniyaTikhomirova
approved these changes
Sep 7, 2026
| const size_t PropByteArraySize = sizeof...(T) * sizeof(uint32_t) * 3; | ||
| const uint64_t PropByteArraySize = sizeof...(T) * sizeof(uint32_t) * 3; | ||
| std::vector<char> DescData; | ||
| DescData.resize(8 + PropByteArraySize); |
Contributor
There was a problem hiding this comment.
I would then replace this "8" with sizeof(PropByteArraySize) since it seems to be related and makes intention more obvious.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
1.
sycl/unittests/helpers/MockDeviceImage.hpp— 8 objects instead of 8 bytesFour places wrote a size header like this:
The intent is "copy 8 bytes", but the operands are
size_t *, so the range spans 8 objects = 64 bytes: an out-of-bounds read of the scalar, and a 64-byte write into a 20-byteDescData.UBSan diagnostic:
Fixed by using
std::memcpywithsizeof, and by makingPropByteArraySizeauint64_tto match the on-disk property format.The same file had a latent under-copy in
makeAspectsProp():std::uninitialized_copy(AspectsPtr, AspectsPtr + Aspects.size(), ...);AspectsPtrisconst unsigned char *, so this copiesAspects.size()bytes instead ofAspects.size() * sizeof(sycl::aspect). Not a sanitizer error (in bounds both ways) and currently harmless because both callers pass a single small-valued aspect, but a multi-aspect test would silently read zeros past the first aspect. The neighbouringmakeReqdWGSizeProp()already multiplies bysizeof(int); this now matches it.2.
sycl/unittests/SYCL2020/KernelBundleStateFiltering.cpp— indexing the wrong pointer*params.pppBinaries[i]parses as*(params.pppBinaries[i]): it indexes the pointer to the argument inside the params struct, not the binaries array, so fori > 0it reads past the struct.ProgramManager::createURProgramfills every entry of that array with the same image pointer, so tracking one entry is both correct and what the code accidentally did before; the loop is dropped in favour of**params.pppBinaries. Indexing correctly instead would double-count images in the two-device case and breakverifyImageUse.Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com