Skip to content

Commit

Permalink
[SYCL] Prevent unintended sign extensions (#15230)
Browse files Browse the repository at this point in the history
This is to resolve a Coverity hit, pointing out that a potentially
unintended sign extension is possible:

- `SectionHeaderNum` and `SectionHeaderSize` are unsigned i16.
- By C++ promotion rules, `SectionHeaderNum * SectionHeaderSize` results
in a signed i32 value.
- However, `SectionHeaderOffset` is i64, so the i32 product is promoted
to i64 via sign extension
- If `SectionHeaderNum * SectionHeaderSize` results in a number greater
than 0x7FFFFF (greatest signed i32 number), then the upper bits get set
to 1, producing the wrong result.

This PR adds a cast to ensure the promotions do not result in unexpected
sign extensions: While I recognize that the odds of `SectionHeaderNum`
and `SectionHeaderSize` getting as big as sqrt(0x7FFFFFF) is rather
unlikely, this change does not hurt or slow down the code, and is
probably the safer thing to do anyway. If it is decided that this change
is not necessarily, feel free to close the PR, and I will fix the
Coverity triage to reflect the decision.
  • Loading branch information
ianayl authored Sep 5, 2024
1 parent 153fab3 commit 1345ae0
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion sycl/source/detail/ur.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,8 @@ static bool checkELFSectionPresent(const std::string &ExpectedSectionName,

// End early if we do not have the expected number of section headers or
// if the read section string header index is out-of-range.
if (ImgSize < SectionHeaderOffset + SectionHeaderNum * SectionHeaderSize ||
if (ImgSize < SectionHeaderOffset + static_cast<uint64_t>(SectionHeaderNum) *
SectionHeaderSize ||
SectionStringsHeaderIndex >= SectionHeaderNum)
return false;

Expand Down

0 comments on commit 1345ae0

Please sign in to comment.