AltF02 / x11-rs

Rust bindings for X11 libraries
https://docs.rs/x11
MIT License
204 stars 66 forks source link

Valgrind reports allocated mem at close #67

Open derekdreery opened 7 years ago

derekdreery commented 7 years ago

I ran valgrind on some code I had written and found that some memory isn't deallocated on close. This probably doesn't matter unless you keep closing/opening the X lib a LOT, but it's nice to tidy up after ourselves ;). I used x11_dl, haven't checked x11.

Of course it might be a leak in X, or some mem that can only be allocated once, in which case it doesn't matter.

Valgrind report

``` ==4119== Memcheck, a memory error detector ==4119== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==4119== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info ==4119== Command: ./target/debug/main ==4119== ==4119== ==4119== HEAP SUMMARY: ==4119== in use at exit: 32 bytes in 1 blocks ==4119== total heap usage: 136 allocs, 135 frees, 83,124 bytes allocated ==4119== ==4119== 32 bytes in 1 blocks are still reachable in loss record 1 of 1 ==4119== at 0x4C2EF35: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==4119== by 0x4E3A63B: ??? (in /usr/lib/libdl-2.26.so) ==4119== by 0x4E39F21: dlopen (in /usr/lib/libdl-2.26.so) ==4119== by 0x148E98: x11_dl::link::DynamicLibrary::open (link.rs:90) ==4119== by 0x149391: x11_dl::link::DynamicLibrary::open_multi (link.rs:119) ==4119== by 0x14A6C6: x11_dl::xlib::Xlib::open (link.rs:57) ==4119== by 0x11C1A8: main::run (main.rs:7) ==4119== by 0x11CBB6: main::main (:4) ==4119== by 0x17E6DC: __rust_maybe_catch_panic (lib.rs:99) ==4119== by 0x17811B: try<(),closure> (panicking.rs:459) ==4119== by 0x17811B: catch_unwind (panic.rs:361) ==4119== by 0x17811B: std::rt::lang_start (rt.rs:59) ==4119== ==4119== LEAK SUMMARY: ==4119== definitely lost: 0 bytes in 0 blocks ==4119== indirectly lost: 0 bytes in 0 blocks ==4119== possibly lost: 0 bytes in 0 blocks ==4119== still reachable: 32 bytes in 1 blocks ==4119== suppressed: 0 bytes in 0 blocks ==4119== ==4119== For counts of detected and suppressed errors, rerun with: -v ==4119== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) ```

derekdreery commented 7 years ago

It's probably caused by ::std::mem::forget(lib); at link.rs:60.

ghost commented 6 years ago

If you still have that code, can you post the valgrind output? That line with forget(lib) shouldn't be causing any problems to my knowledge. The swap just before it ensures that lib is null, and forget is called just to prevent the destructor from being called. It's not a nice way to accomplish what I wanted, but I wrote that code when I was new to Rust <.<

Sorry for the delayed response. I haven't had time for x11-dl since I started going back to college, and I don't have anyone else to maintain it for me.

derekdreery commented 6 years ago

Hi, no problem! The output is in the <details> tag, unless you mean something else?

ghost commented 6 years ago

Whoops! I guess I missed that.

Digging through this, I'm noticing how ugly my old code is and remembering that I was rushing to get it working at the time, but in the process, I remembered something that I was fighting with when I wrote the code.

There was an issue somebody opened a long time ago in which Xlib::open() (for each library struct) was crasing on some systems because it was using an absurd amount of stack memory, so I changed the function in a way that guarantees that only one copy of the struct could possibly be allocated on the stack. The init function initializes the members by taking a pointer to the huge struct instead of returning an instance on success, as you would normally do in Rust.

While I was making this change, something strange happened. I wrote the destructor for DynamicLibrary to check if the handle was null before calling dlclose, which seemed like a simple solution to a simple problem at the time. The only member in DynamicLibrary is the handle, so an instance initialized with mem::zeroed() shouldn't cause any problems. But for some reason it did. The very simple destructor for the very simple struct was crashing, and to this day, I still have no idea why.

I used mem::swap() and mem::forget() to get around this weird problem. There are two instances of DynamicLibrary: one within the library struct and a temporary one. The one inside the struct gets a valid handle which should be cleaned up with dlclose when it is dropped, while the temporary one (which should be null after the swap) goes away with mem::forget() to prevent the somehow crashing destructor from being called.

Judging by the valgrind output, it looks like dlclose isn't being called. In my code in another project, the only Xlib struct instantiated is inside a lazy_static, so you would expect it to never be destroyed, but if Xlib is dropped, then I don't see any reason that dlclose would not be called. This is unsatisfying to say, but I have no idea why the code isn't working (or why the destructor used to crash in the old code before I push the current code). Maybe there's something obvious going on that I'm just missing for some reason.

derekdreery commented 6 years ago

I'm not sure it's such a major problem to be honest, maybe just leave the issue open?