Skip to content

Commit ad67cf4

Browse files
kripkenkripken
authored
Binary Reading: Avoid overlapping internal names between imports and non-imports (#7700)
We handled name overlaps (name section name that happens to collide with the next internal name we invent for a non-named thing), but we did it separately for imports and non-imports, allowing a collision between them in very odd situations. To fix that, maintain a single shared list of used names for imports and non-imports.
1 parent c3f7e2e commit ad67cf4

File tree

3 files changed

+44
-21
lines changed

3 files changed

+44
-21
lines changed

‎src/wasm-binary.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1658,6 +1658,11 @@ class WasmBinaryReader {
16581658
std::unordered_map<Index, Name> dataNames;
16591659
std::unordered_map<Index, Name> elemNames;
16601660

1661+
// The names that are already used (either from the names section, or that we
1662+
// generate as internal names for un-named things).
1663+
std::unordered_set<Name> usedFunctionNames, usedTableNames, usedMemoryNames,
1664+
usedGlobalNames, usedTagNames;
1665+
16611666
Function* currFunction = nullptr;
16621667
// before we see a function (like global init expressions), there is no end of
16631668
// function to check

‎src/wasm/wasm-binary.cpp

Lines changed: 14 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2529,17 +2529,15 @@ getOrMakeName(const std::unordered_map<Index, Name>& nameMap,
25292529
void WasmBinaryReader::readMemories() {
25302530
auto num = getU32LEB();
25312531
auto numImports = wasm.memories.size();
2532-
std::unordered_set<Name> usedNames;
25332532
for (auto& [index, name] : memoryNames) {
25342533
if (index >= num + numImports) {
25352534
std::cerr << "warning: memory index out of bounds in name section: "
25362535
<< name << " at index " << index << '\n';
25372536
}
2538-
usedNames.insert(name);
25392537
}
25402538
for (size_t i = 0; i < num; i++) {
2541-
auto [name, isExplicit] =
2542-
getOrMakeName(memoryNames, numImports + i, makeName("", i), usedNames);
2539+
auto [name, isExplicit] = getOrMakeName(
2540+
memoryNames, numImports + i, makeName("", i), usedMemoryNames);
25432541
auto memory = Builder::makeMemory(name);
25442542
memory->hasExplicitName = isExplicit;
25452543
getResizableLimits(memory->initial,
@@ -2871,8 +2869,6 @@ void WasmBinaryReader::getResizableLimits(Address& initial,
28712869
void WasmBinaryReader::readImports() {
28722870
size_t num = getU32LEB();
28732871
Builder builder(wasm);
2874-
std::unordered_set<Name> usedFunctionNames, usedTableNames, usedMemoryNames,
2875-
usedGlobalNames, usedTagNames;
28762872
for (size_t i = 0; i < num; i++) {
28772873
auto module = getInlineString();
28782874
auto base = getInlineString();
@@ -3007,13 +3003,11 @@ void WasmBinaryReader::setLocalNames(Function& func, Index i) {
30073003
void WasmBinaryReader::readFunctionSignatures() {
30083004
size_t num = getU32LEB();
30093005
auto numImports = wasm.functions.size();
3010-
std::unordered_set<Name> usedNames;
30113006
for (auto& [index, name] : functionNames) {
30123007
if (index >= num + numImports) {
30133008
std::cerr << "warning: function index out of bounds in name section: "
30143009
<< name << " at index " << index << '\n';
30153010
}
3016-
usedNames.insert(name);
30173011
}
30183012
// Also check that the function indices in the local names subsection are
30193013
// in-bounds, even though we don't use them here.
@@ -3025,8 +3019,8 @@ void WasmBinaryReader::readFunctionSignatures() {
30253019
}
30263020
}
30273021
for (size_t i = 0; i < num; i++) {
3028-
auto [name, isExplicit] =
3029-
getOrMakeName(functionNames, numImports + i, makeName("", i), usedNames);
3022+
auto [name, isExplicit] = getOrMakeName(
3023+
functionNames, numImports + i, makeName("", i), usedFunctionNames);
30303024
auto index = getU32LEB();
30313025
HeapType type = getTypeByIndex(index);
30323026
functionTypes.push_back(type);
@@ -4761,17 +4755,15 @@ Name WasmBinaryReader::getIndexedString() {
47614755
void WasmBinaryReader::readGlobals() {
47624756
size_t num = getU32LEB();
47634757
auto numImports = wasm.globals.size();
4764-
std::unordered_set<Name> usedNames;
47654758
for (auto& [index, name] : globalNames) {
47664759
if (index >= num + numImports) {
47674760
std::cerr << "warning: global index out of bounds in name section: "
47684761
<< name << " at index " << index << '\n';
47694762
}
4770-
usedNames.insert(name);
47714763
}
47724764
for (size_t i = 0; i < num; i++) {
47734765
auto [name, isExplicit] = getOrMakeName(
4774-
globalNames, numImports + i, makeName("global$", i), usedNames);
4766+
globalNames, numImports + i, makeName("global$", i), usedGlobalNames);
47754767
auto type = getConcreteType();
47764768
auto mutable_ = getU32LEB();
47774769
if (mutable_ & ~1) {
@@ -4860,17 +4852,15 @@ void WasmBinaryReader::readDataSegments() {
48604852
void WasmBinaryReader::readTableDeclarations() {
48614853
auto num = getU32LEB();
48624854
auto numImports = wasm.tables.size();
4863-
std::unordered_set<Name> usedNames;
48644855
for (auto& [index, name] : tableNames) {
48654856
if (index >= num + numImports) {
48664857
std::cerr << "warning: table index out of bounds in name section: "
48674858
<< name << " at index " << index << '\n';
48684859
}
4869-
usedNames.insert(name);
48704860
}
48714861
for (size_t i = 0; i < num; i++) {
4872-
auto [name, isExplicit] =
4873-
getOrMakeName(tableNames, numImports + i, makeName("", i), usedNames);
4862+
auto [name, isExplicit] = getOrMakeName(
4863+
tableNames, numImports + i, makeName("", i), usedTableNames);
48744864
auto elemType = getType();
48754865
if (!elemType.isRef()) {
48764866
throwError("Table type must be a reference type");
@@ -4977,18 +4967,16 @@ void WasmBinaryReader::readElementSegments() {
49774967
void WasmBinaryReader::readTags() {
49784968
size_t num = getU32LEB();
49794969
auto numImports = wasm.tags.size();
4980-
std::unordered_set<Name> usedNames;
49814970
for (auto& [index, name] : tagNames) {
49824971
if (index >= num + numImports) {
49834972
std::cerr << "warning: tag index out of bounds in name section: " << name
49844973
<< " at index " << index << '\n';
49854974
}
4986-
usedNames.insert(name);
49874975
}
49884976
for (size_t i = 0; i < num; i++) {
49894977
getInt8(); // Reserved 'attribute' field
4990-
auto [name, isExplicit] =
4991-
getOrMakeName(tagNames, numImports + i, makeName("tag$", i), usedNames);
4978+
auto [name, isExplicit] = getOrMakeName(
4979+
tagNames, numImports + i, makeName("tag$", i), usedTagNames);
49924980
auto typeIndex = getU32LEB();
49934981
auto tag = Builder::makeTag(name, getSignatureByTypeIndex(typeIndex));
49944982
tag->hasExplicitName = isExplicit;
@@ -5081,6 +5069,7 @@ void WasmBinaryReader::readNames(size_t sectionPos, size_t payloadLen) {
50815069
auto rawName = getInlineString();
50825070
auto name = processor.process(rawName);
50835071
functionNames[index] = name;
5072+
usedFunctionNames.insert(name);
50845073
}
50855074
} else if (nameType == Subsection::NameLocal) {
50865075
auto numFuncs = getU32LEB();
@@ -5112,6 +5101,7 @@ void WasmBinaryReader::readNames(size_t sectionPos, size_t payloadLen) {
51125101
auto rawName = getInlineString();
51135102
auto name = processor.process(rawName);
51145103
tableNames[index] = name;
5104+
usedTableNames.insert(name);
51155105
}
51165106
} else if (nameType == Subsection::NameElem) {
51175107
auto num = getU32LEB();
@@ -5130,6 +5120,7 @@ void WasmBinaryReader::readNames(size_t sectionPos, size_t payloadLen) {
51305120
auto rawName = getInlineString();
51315121
auto name = processor.process(rawName);
51325122
memoryNames[index] = name;
5123+
usedMemoryNames.insert(name);
51335124
}
51345125
} else if (nameType == Subsection::NameData) {
51355126
auto num = getU32LEB();
@@ -5148,6 +5139,7 @@ void WasmBinaryReader::readNames(size_t sectionPos, size_t payloadLen) {
51485139
auto rawName = getInlineString();
51495140
auto name = processor.process(rawName);
51505141
globalNames[index] = name;
5142+
usedGlobalNames.insert(name);
51515143
}
51525144
} else if (nameType == Subsection::NameField) {
51535145
auto numTypes = getU32LEB();
@@ -5170,6 +5162,7 @@ void WasmBinaryReader::readNames(size_t sectionPos, size_t payloadLen) {
51705162
auto rawName = getInlineString();
51715163
auto name = processor.process(rawName);
51725164
tagNames[index] = name;
5165+
usedTagNames.insert(name);
51735166
}
51745167
} else {
51755168
std::cerr << "warning: unknown name subsection with id "

‎test/lit/name-overlap.wast

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
2+
3+
;; RUN: foreach %s %t wasm-opt -all --instrument-branch-hints --roundtrip -S -o - | filecheck %s
4+
5+
;; Two imports exist here, and instrument-branch-hints will add another. The
6+
;; name "fimport$2" happens to be the name that would be chosen for that new
7+
;; import, leading to a situation that the existing import has a forced name
8+
;; from the names section (it is named here in the wat) while we pick an
9+
;; internal name (not from the name section) that overlaps with it, causing an
10+
;; error if we do not make sure to avoid duplication between import and non-
11+
;; import names.
12+
13+
(module
14+
;; CHECK: (type $0 (func (param i64)))
15+
16+
;; CHECK: (type $1 (func (param f32)))
17+
18+
;; CHECK: (type $2 (func (param i32 i32 i32)))
19+
20+
;; CHECK: (import "fuzzing-support" "log-i64" (func $fimport$2 (type $0) (param i64)))
21+
(import "fuzzing-support" "log-i64" (func $fimport$2 (param i64)))
22+
;; CHECK: (import "fuzzing-support" "log-f32" (func $fimport$3 (type $1) (param f32)))
23+
(import "fuzzing-support" "log-f32" (func $fimport$3 (param f32)))
24+
)
25+
;; CHECK: (import "fuzzing-support" "log-branch" (func $fimport$2_2 (type $2) (param i32 i32 i32)))

0 commit comments

Comments
 (0)