Skip to content

spirv: fix binary encoding/decoding on big-endian hosts #3658

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
amaanq wants to merge 1 commit into
base: main
Choose a base branch
from
+21 −7
Open

spirv: fix binary encoding/decoding on big-endian hosts #3658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions lib/SPIRV/libSPIRV/SPIRVModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2623,7 +2623,8 @@ std::istream &SPIRVModuleImpl::parseSPIRV(std::istream &I) {
MI.setAutoAddExtensions(false);

SPIRVWord Header[5] = {0};
I.read(reinterpret_cast<char *>(&Header), sizeof(Header));
for (auto &H : Header)
H = readSPIRVWord(I);

SPIRVErrorLog &ErrorLog = MI.getErrorLog();
if (!ErrorLog.checkError(!I.eof(), SPIRVEC_InvalidModule,
Expand Down Expand Up @@ -2660,8 +2661,7 @@ std::istream &SPIRVModuleImpl::parseSPIRV(std::istream &I) {

SPIRVEntry *Scope = nullptr;
while (true) {
SPIRVWord WordCountAndOpCode = 0;
I.read(reinterpret_cast<char *>(&WordCountAndOpCode), sizeof(SPIRVWord));
SPIRVWord WordCountAndOpCode = readSPIRVWord(I);
SPIRVDBG(spvdbgs() << "Read word: W = " << WordCountAndOpCode
<< " V = 0\n");
SPIRVWord WordCount = WordCountAndOpCode >> 16;
Expand Down Expand Up @@ -2784,8 +2784,8 @@ SPIRVId SPIRVModuleImpl::getExtInstSetId(SPIRVExtInstSetKind Kind) const {
bool isSpirvBinary(const std::string &Img) {
if (Img.size() < sizeof(unsigned))
return false;
const auto *Magic = reinterpret_cast<const unsigned *>(Img.data());
return *Magic == MagicNumber;
std::istringstream IS(Img);
return readSPIRVWord(IS) == MagicNumber;
}
Comment on lines 2784 to 2789
Copy link
Contributor

@MrSidims MrSidims Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand the code correctly, as isSpirvBinary is public API - Img can be the whole SPIR-V module passed in a library call. If I'm right, I'd like to avoid unnecessary copy of Img. I'd suggest to avoid it either by memcopying only 4 bytes or byte checking first 4 bytes byte-by-byte.


#ifdef _SPIRV_SUPPORT_TEXT_FMT
Expand Down
18 changes: 16 additions & 2 deletions lib/SPIRV/libSPIRV/SPIRVStream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,20 @@ class SPIRVNL {
friend spv_ostream &operator<<(spv_ostream &O, const SPIRVNL &E);
};

/// Read a single SPIR-V word from a binary stream, byte-swapping from
/// little-endian on big-endian hosts.
inline uint32_t readSPIRVWord(std::istream &IS) {
uint32_t W;
Copy link
Contributor

@MrSidims MrSidims Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets fix this while we are here as with this patch this may become critical parser vulnerability for attacker-controlled input as we start to use decodeBinary/readSPIRVWord to parse the header

Suggested change
uint32_t W;
uint32_t W = 0;

IS.read(reinterpret_cast<char *>(&W), sizeof(W));
#if defined(__BYTE_ORDER__) && __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
W = __builtin_bswap32(W);
#endif
return W;
}

template <typename T>
const SPIRVDecoder &decodeBinary(const SPIRVDecoder &I, T &V) {
uint32_t W;
I.IS.read(reinterpret_cast<char *>(&W), sizeof(W));
uint32_t W = readSPIRVWord(I.IS);
V = static_cast<T>(W);
SPIRVDBG(spvdbgs() << "Read word: W = " << W << " V = " << V << '\n');
return I;
Expand Down Expand Up @@ -186,6 +196,10 @@ const SPIRVEncoder &operator<<(const SPIRVEncoder &O, T V) {
}
#endif
uint32_t W = static_cast<uint32_t>(V);
#if defined(__BYTE_ORDER__) && __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
// SPIR-V binary format is little-endian; byte-swap on big-endian hosts.
W = __builtin_bswap32(W);
#endif
O.OS.write(reinterpret_cast<char *>(&W), sizeof(W));
return O;
}
Expand Down