Skip to content

fix: Protect mem consumption from null deref#4895

Open
lum1n0us wants to merge 1 commit intobytecodealliance:mainfrom
lum1n0us:fix/memory-consumption-null-deref
Open

fix: Protect mem consumption from null deref#4895
lum1n0us wants to merge 1 commit intobytecodealliance:mainfrom
lum1n0us:fix/memory-consumption-null-deref

Conversation

@lum1n0us
Copy link
Copy Markdown
Contributor

  • add debug assertions to wasm_get_module_inst_mem_consumption
  • add debug assertions to aot_get_module_inst_mem_consumption
  • nullify memories pointer after free in destruction path
  • nullify memories pointer after free in AOT destruction
  • docs: add precondition documentation for memory consumption functions

- add debug assertions to wasm_get_module_inst_mem_consumption
- add debug assertions to aot_get_module_inst_mem_consumption
- nullify memories pointer after free in destruction path
- nullify memories pointer after free in AOT destruction
- docs: add precondition documentation for memory consumption functions
if (module_inst->memory_count > 0) {
memories_deinstantiate(module_inst, module_inst->memories,
module_inst->memory_count);
module_inst->memories = NULL;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think it's worth setting NULL here? Given the fact that this module inst is about to be destroyed anyway? It will create an inconsistency with other data members, like L3455

}
}
wasm_runtime_free(module_inst->memories);
module_inst->memories = NULL;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think it's worth setting NULL here? Given the fact that the module instance will be destroyed later anyway?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants