Skip to content

Trap on argument count mismatch in DefinedFunc::DoCall #2866

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
aizu-m wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Trap on argument count mismatch in DefinedFunc::DoCall #2866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5c7478
Trap on argument count mismatch in DefinedFunc::DoCall
aizu-m Sep 27, 2026
15a8333
Reuse the existing fac module in the arity mismatch test
aizu-m Oct 1, 2026
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
6 changes: 4 additions & 2 deletions src/interp/interp-util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include "wabt/interp/interp-util.h"

#include <algorithm>
#include <cinttypes>

#include "wabt/stream.h"
Expand Down Expand Up @@ -84,9 +85,10 @@ void WriteValues(Stream* stream,
const ValueTypes& types,
const Values& values) {
assert(types.size() == values.size());
for (size_t i = 0; i < values.size(); ++i) {
size_t count = std::min(types.size(), values.size());
for (size_t i = 0; i < count; ++i) {
WriteValue(stream, TypedValue{types[i], values[i]});
if (i != values.size() - 1) {
if (i != count - 1) {
stream->Writef(", ");
}
}
Expand Down
10 changes: 9 additions & 1 deletion src/interp/interp.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,15 @@ Result DefinedFunc::DoCall(Thread& thread,
const Values& params,
Values& results,
Trap::Ptr* out_trap) {
assert(params.size() == type_.params.size());
if (params.size() != type_.params.size()) {

@zherczeg zherczeg Sep 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this is the hot path? I mean every call goes through this path? If this affect user code only, maybe adding something higher level would be better. I know this interpreter is testing only, but still.

@aizu-m aizu-m Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not the hot path. Traced it before adding the check:

  • in-wasm call, call_indirect, call_ref and return_call* all route through Thread::DoCall (interp.cc:2198). For a DefinedFunc that only does PushCall, so it never enters DefinedFunc::DoCall.
  • DefinedFunc::DoCall is reached solely from the two Func::Call overloads, i.e. the embedder boundary. In tree that's spectest-interp's RunAction, wasm-interp, the C API and the wasi start call.

So it costs one size comparison per external call, next to a Thread construction and a params copy, and nothing per in-wasm call. Behaviour inside the interpreter loop is untouched.

On moving it higher: DefinedFunc::DoCall is already the single funnel for both Func::Call overloads. Putting the check in Func::Call would mean duplicating it across both (the Thread& overload is the one re-entrant host callbacks use) or leaving one unguarded. Happy to move it up if you'd prefer that shape.

*out_trap =
Trap::New(thread.store(),
StringPrintf("argument count mismatch: expected %" PRIindex
", got %" PRIindex,
static_cast<Index>(type_.params.size()),
static_cast<Index>(params.size())));
return Result::Error;
}
thread.PushValues(type_.params, params);
RunResult result = thread.PushCall(*this, out_trap);
if (result == RunResult::Trap) {
Expand Down
15 changes: 15 additions & 0 deletions src/test-interp.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,21 @@ TEST_F(InterpTest, Fac) {
EXPECT_EQ(120u, results[0].Get<u32>());
}

TEST_F(InterpTest, CallArgCountMismatch) {
ReadModule(s_fac_module);
Instantiate();
auto func = GetFuncExport(0);

// Calling the one-parameter export with no arguments must trap rather than
// read past the end of the params vector.
Values results;
Trap::Ptr trap;
Result result = func->Call(store_, {}, results, &trap);

ASSERT_EQ(Result::Error, result);
ASSERT_TRUE(trap);

@sbc100 sbc100 Sep 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can this test not be written as a normal text-based test?

Binary encoded tests like these should be avoided where possible I think.

@aizu-m aizu-m Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Tried that first, but a text spec test can't reach this path. wast2json validates invoke arity, so (invoke "f") against a one-param export is rejected at parse time (too few parameters to function. got 0, expected 1) and never reaches spectest-interp. The too-many-args case that hits WriteValues is rejected the same way. The over-read only shows up once that parser check is bypassed, i.e. through the Func::Call C-API directly or a hand-written JSON, so the gtest is the only thing that exercises it. It also follows the existing binary-module style in test-interp.cc. Happy to drop the test and land just the fix if you'd rather avoid the binary blob.

@aizu-m aizu-m Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Dropped the new blob. The test now reuses the existing s_fac_module, which is already a one-param (param i32) (result i32) export, so there's no new binary encoding in the diff.

Re-checked the text route before doing that. wast2json still rejects (invoke "f") against that export at parse time (too few parameters to function. got 0, expected 1), and run-tests.py only reaches spectest-interp by way of wast2json, so there's no checked-in-JSON path to drive a short call through. The gtest stays the only place the over-read is reachable.

}

TEST_F(InterpTest, Fac_Trace) {
ReadModule(s_fac_module);
Instantiate();
Expand Down
Loading