icedland / iced

Blazing fast and correct x86/x64 disassembler, assembler, decoder, encoder for Rust, .NET, Java, Python, Lua
MIT License
2.91k stars 232 forks source link

Using Box::into_raw has resulted in numerous memory leak issues #552

Closed persytry closed 5 months ago

persytry commented 5 months ago

image image There are numerous instances where the Box::into_raw method is being used, which can lead to memory leaks. For example, within the box_opcode_handler function, Box::into_raw is employed, returning a *const OpCodeHandler. This specific usage results in a memory leak. Given that the box_opcode_handler function is called from many locations throughout the codebase, it follows that iced-x86 suffers from severe memory leak problems. We kindly request that you address and rectify these issues. Thank you.

wtfsck commented 5 months ago

I think this has been fixed already if you try the latest master.

They're all stored in lazy_statics.

persytry commented 5 months ago

I use the iced-x86 version "1.21.0", and also git clone the lastest master of iced, both has the leaks issues. You should look the function box_opcode_handler(iced/src/rust/iced-x86/src/decoder/table_de/mod.rs):

fn box_opcode_handler<T>((decode, handler): (OpCodeHandlerDecodeFn, T)) -> (OpCodeHandlerDecodeFn, *const OpCodeHandler) {
    // All handlers are #[repr(C)] and the first fields are the same as OpCodeHandler
    (decode, Box::into_raw(Box::new(handler)) as *const OpCodeHandler)
}

The usage is wrong above: Box::into_raw(Box::new(handler)) as *const OpCodeHandler. Can not be used like that, it causes leaks. If you use the function Box::into_raw" , you must useBox::from_raw` also, or else would be leaks. Please fix it, iced-x86 has many leaks like that.

persytry commented 5 months ago

I think this has been fixed already if you try the latest master.

They're all stored in lazy_statics.

The results of function box_opcode_handler, has not stored in lazy_statics

wtfsck commented 5 months ago

Do you have a repro using the latest master? This has already been talked about in #524

The results of function box_opcode_handler, has not stored in lazy_statics

They're reachable from lazy statics.

persytry commented 5 months ago

This is my Cargo.toml: image I used the lastest master, and has the leaks too. I found two cases cause leaks:

  1. use Box::into_raw, but not use Box::from_raw to free the memory.
  2. usage of lazy_static is wrong, because iced-x86 not support function to free memories stored in lazy_static by manual, look this article.

iced-x86 should support some functions to free all memories which allocated by iced-x86, example allocated by Box::into_raw and lazy_static. I develop windows driver(kernel module) program, I must control all memory malloc and free by mannual, so iced-x86 should support it. When I unload my driver program, I must free all memory, but I found iced-x86 leaks. Thanks.

wtfsck commented 5 months ago

Memory allocated by lazy statics are supposed to last the whole program execution and thus they get freed when the program exits.

use Box::into_raw, but not use Box::from_raw to free the memory.

Because they're lazy statics and they're not meant to be freed.

usage of lazy_static is wrong, because iced-x86 not support function to free memories stored in lazy_static by manual, look this article.

It's not wrong because I don't use a feature to free lazy static memory. They're supposed to be there the whole time the program is executed.

I do have plans to remove the lazy static dependency in a future version though.

Do you have a repro of the memory leak or do you consider it to be a memory leak because I use lazy statics that are freed on program exit?

persytry commented 5 months ago

Memory allocated by lazy statics are supposed to last the whole program execution and thus they get freed when the program exits.

You said the whole program only means "the user mode program", which run system ring-3 level. lazy_static could freed memory at ring-3 program, because it depend on CRT runtime. But CRT runtime only works on ring-3 level program, not work in kernel model program. So lazy_static will not free memory automatically at ring-0 kernel mode program. iced-x86 support no_std, the ring-0 kernel mode program(e.g. windows .sys file, the driver program) only could run at no_std environment. And no_std environment has not CRT runtime. A driver program like a dll, which all can be load and unload. My Driver use iced-x86 at loading time, when I unload my driver, I free iced-x86 and free all memory by manually, because lazy_static can not free automatically, it is not like user mode program. That is determined by windows system mechanism, user mode program runtime mode is not same as kernel mode program.

persytry commented 5 months ago

Enven if iced-x86 only run at user mode program, your Box::into_raw and lazy_static usage is wrong. First, iced-x86 should not use lazy_static, should not stored memories in lazy_static which has whole program life time, it's not reasonable. I maybe use iced-x86 once at my user mode program, but it use memory last whole program life time. I maybe use iced-x86 many times at my user mode program, I would found my memory is keeping growing. Second, lazy_static only free memory it maintains itself, would not free the memories allocated by Box::new.

persytry commented 5 months ago
  1. I suggest you remove the lazy_static dependency, otherwise iced-x86 could not support no_std well.
  2. I suggest the memories allocated by Box::new, please freed them by Box::from_raw.
  3. I suggest don't depend on the program exit to free all memories, because iced-x86 is a common base library, is not a application.
wtfsck commented 5 months ago

This is not a memory leak since it's stored in a lazy static and statics are by definition only freed once the program exits.

My plan is to remove the lazy static dependency in a future version though.

Closing since this is not a memory leak.

persytry commented 5 months ago

Do you understand? In user mode program, it's maybe not a leaks, but in kernel mode program, it's a leaks issues.