Skip to content

Fix fuzzer generation of a DataSegment + add validation that would have caught it #6626

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

Merged
kripken merged 8 commits into from
May 23, 2024
Merged

Fix fuzzer generation of a DataSegment + add validation that would have caught it #6626

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
20ce9d3
start
kripken May 23, 2024
1d6dde2
work
kripken May 23, 2024
195d558
bettr
kripken May 23, 2024
a16b5d6
bettr
kripken May 23, 2024
fc3061a
fix
kripken May 23, 2024
c4f3913
fix name collision
kripken May 23, 2024
617412e
Update src/wasm/wasm-validator.cpp
kripken May 23, 2024
cc68a61
another
kripken May 23, 2024
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
7 changes: 4 additions & 3 deletions src/tools/fuzzing/fuzzing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,13 +224,14 @@ void TranslateToFuzzReader::setupMemory() {
auto segment = builder.makeDataSegment();
segment->memory = memory->name;
segment->offset = builder.makeConst(int32_t(0));
segment->setName(Name::fromInt(0), false);
wasm.dataSegments.push_back(std::move(segment));
segment->setName(Names::getValidDataSegmentName(wasm, Name::fromInt(0)),
false);
auto num = upTo(USABLE_MEMORY * 2);
for (size_t i = 0; i < num; i++) {
auto value = upTo(512);
wasm.dataSegments[0]->data.push_back(value >= 256 ? 0 : (value & 0xff));
Copy link
Copy Markdown
Member

@aheejin aheejin May 23, 2024

Choose a reason for hiding this comment

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

Was this meant to be wasm.dataSegment[i]? It looks it should've been that way if the semantics are the same with the modified code..

Copy link
Copy Markdown
Member Author

@kripken kripken May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, I think it is pushing num chunks of binary data to the first segment here. wasm.dataSegments[i] might not exist - all we know is that [0] exists.

What do you mean by the second sentence?

Copy link
Copy Markdown
Member

@aheejin aheejin May 23, 2024

Choose a reason for hiding this comment

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

Sorry I was confused.. Nevermind.

segment->data.push_back(value >= 256 ? 0 : (value & 0xff));
}
wasm.addDataSegment(std::move(segment));
}
}

Expand Down
58 changes: 56 additions & 2 deletions src/wasm/wasm-validator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3908,7 +3908,7 @@ static void validateTags(Module& module, ValidationInfo& info) {
}
}

static void validateModule(Module& module, ValidationInfo& info) {
static void validateStart(Module& module, ValidationInfo& info) {
// start
if (module.start.is()) {
auto func = module.getFunctionOrNull(module.start);
Expand All @@ -3924,6 +3924,59 @@ static void validateModule(Module& module, ValidationInfo& info) {
}
}

namespace {
template<typename T, typename U>
void validateModuleMap(Module& module,
ValidationInfo& info,
T& list,
U getter,
const std::string& kind) {
// Given a list of module elements (like exports or globals), see that we can
// get the items using the getter (getExportorNull, etc.). The getter uses the
// lookup map internally, so this validates that they contain all items in
// the list.
for (auto& item : list) {
auto* ptr = (module.*getter)(item->name);
if (!ptr) {
info.fail(kind + " must be found (use updateMaps)", item->name, nullptr);
} else {
info.shouldBeEqual(item->name,
ptr->name,
item->name,
"getter must return the correct item");
}
}

// TODO: Also check there is nothing extraneous in the map, but that would
// require inspecting private fields of Module.
}
} // anonymous namespace

static void validateModuleMaps(Module& module, ValidationInfo& info) {
// Module maps should be up to date.
validateModuleMap(
module, info, module.exports, &Module::getExportOrNull, "Export");
validateModuleMap(
module, info, module.functions, &Module::getFunctionOrNull, "Function");
validateModuleMap(
module, info, module.globals, &Module::getGlobalOrNull, "Global");
validateModuleMap(module, info, module.tags, &Module::getTagOrNull, "tag");
validateModuleMap(module,
info,
module.elementSegments,
&Module::getElementSegmentOrNull,
"elementSegment");
validateModuleMap(
module, info, module.memories, &Module::getMemoryOrNull, "Memory");
validateModuleMap(module,
info,
module.dataSegments,
&Module::getDataSegmentOrNull,
"DataSegment");
validateModuleMap(
module, info, module.tables, &Module::getTableOrNull, "Table");
}

static void validateFeatures(Module& module, ValidationInfo& info) {
if (module.features.hasGC()) {
info.shouldBeTrue(module.features.hasReferenceTypes(),
Expand Down Expand Up @@ -4004,7 +4057,8 @@ bool WasmValidator::validate(Module& module, Flags flags) {
validateDataSegments(module, info);
validateTables(module, info);
validateTags(module, info);
validateModule(module, info);
validateStart(module, info);
validateModuleMaps(module, info);
validateFeatures(module, info);
if (info.closedWorld) {
validateClosedWorldInterface(module, info);
Expand Down