Skip to content

Module.execute does not check input count #10510

@GregoryComer

Description

@GregoryComer

🐛 Describe the bug

The execute function in module.cpp will cause silent memory corruption if too many input EValues are passed.

runtime::Result<std::vector<runtime::EValue>> Module::execute(
const std::string& method_name,
const std::vector<runtime::EValue>& input_values) {
ET_CHECK_OK_OR_RETURN_ERROR(load_method(method_name));
auto& method = methods_.at(method_name).method;
auto& inputs = methods_.at(method_name).inputs;
for (size_t i = 0; i < input_values.size(); ++i) {
if (!input_values[i].isNone()) {
inputs[i] = input_values[i];
}
}

If input_values.size() > inputs.size(), the indexing operation on line 242 will write past the end of the array. I encountered this when I updated a model definition but hadn't updated the C++ runner code yet and it caused a memory-related crash when destructing the module.

We should check for input_values.size() <= inputs.size() and log an error + return an error code (Error::InvalidArgument) if the check fails.

As an aside, passing fewer inputs in may or may not be a desirable feature, given that some may be memory planned or unchanged between calls. We can ignore this case for the sake of this bug as it's not a correctness issue, but it might be good to error out for clarity if too few inputs are passed.

Versions

Repro-able on main, though it's likely been like this for a long time (or forever).

Metadata

Metadata

Assignees

Labels

Type

Projects

Status

No status

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions