[examples] Should check import types

[rene-fonseca]

The wasm_instance_new C-API implementation fails due to buffer overrun for the imports buffer.

The imports argument doesn't have a size.

wasm-c-api/src/wasm-c.cc

Lines 995 to 1006 in b0b4bfb

	wasm_instance_t* wasm_instance_new(
	wasm_store_t* store,
	const wasm_module_t* module,
	const wasm_extern_t* const imports[],
	wasm_trap_t** trap
	) {
	own<Trap> error;
	auto instance = release_instance(Instance::make(store, module,
	reinterpret_cast<const Extern* const*>(imports), &error));
	if (trap) *trap = hide_trap(error.release());
	return instance;
	}

Since there is no size given as argument, it needs to be assumed that the imports buffer is NULL terminated. Note that all C-API examples need to be updated to add the NULL as the last item for the imports buffer.

[rossberg]

This seems to be a bug with Wasmtime's implementation of the C API, not the C API definition itself (this repo). Can you please file it against Wasmtime?

[rene-fonseca]

Did make it originally for wasmtime :)

I'll reopen that @yurydelendik.

[yurydelendik]

The snippet from wasmtime is irrelevant for the issue: here is v8 related snippet:

wasm-c-api/src/wasm-v8.cc

Lines 2036 to 2062 in b0b4bfb

	for (size_t i = 0; i < import_types.size(); ++i) {
	auto type = import_types[i].get();
	auto maybe_module = v8::String::NewFromOneByte(
	isolate, reinterpret_cast<const uint8_t*>(type->module().get()),
	v8::NewStringType::kNormal, type->module().size()
	);
	if (maybe_module.IsEmpty()) return own<Instance>();
	auto module_str = maybe_module.ToLocalChecked();
	auto maybe_name = v8::String::NewFromOneByte(
	isolate, reinterpret_cast<const uint8_t*>(type->name().get()),
	v8::NewStringType::kNormal, type->name().size()
	);
	if (maybe_name.IsEmpty()) return own<Instance>();
	auto name_str = maybe_name.ToLocalChecked();
	v8::Local<v8::Object> module_obj;
	if (imports_obj->HasOwnProperty(context, module_str).ToChecked()) {
	module_obj = v8::Local<v8::Object>::Cast(
	imports_obj->Get(context, module_str).ToLocalChecked());
	} else {
	module_obj = v8::Object::New(isolate);
	ignore(imports_obj->DefineOwnProperty(context, module_str, module_obj));
	}
	ignore(module_obj->DefineOwnProperty(
	context, name_str, extern_to_v8(imports[i])));
	}

[rene-fonseca]

I updated the initial description also to apply to this repo.

[rene-fonseca]

Seems risky to rely on caller-only for ensuring that imports match.

Would be good to update examples to include such validation. E.g.

wasm-c-api/example/callback.c

Lines 111 to 116 in 7865f7d

	const wasm_extern_t* imports[] = {
	wasm_func_as_extern(print_func), wasm_func_as_extern(closure_func)
	};
	own wasm_instance_t* instance =
	wasm_instance_new(store, module, imports, NULL);
	if (!instance) {

I didn't spot import validation in the examples I looked through.

module would commonly be a file on disk - and if that was changed - it can cause undefined behavior/crashing - unless there is code to catch the mismatch.

[rossberg]

Seems risky to rely on caller-only for ensuring that imports match.

Well, that's how C works just about everywhere. There is no safety in C.

But fair point about the examples, they aren't as defensive as they should be in the real world. I changed the subject of this issue to reflect that as a todo.

[peterhuene]

For what it's worth, I filed a similar issue (#104) when I first encountered the C API, but I've come around that adding a size argument would be redundant as it must always match the length of the module's imports and it isn't guaranteed in C to accurately describe the array being passed in anyway.