search

rust-x-bindings / rust-xcb

Rust bindings and wrapper for XCB.
MIT License
165 stars 64 forks source link

Memory leak when requesting a property without calling `get_reply()` #57

Closed daniel-abramov closed 4 years ago

daniel-abramov commented 6 years ago

It seems that if we request the property via xcb::xproto::get_property(), we must call get_reply(), otherwise the corresponding memory will not be freed and the memory leak will happen. I suspect that happens because get_reply() returns a xcb::base::Reply<T> instance which implements Drop which in turn calls libc::free() to deallocate the memory. However if get_reply() is not called, it always leads to a memory leak.

psychon commented 5 years ago

Since this doesn't build currently for me, I cannot test this, but I propose to implement Drop for base::Cookie by calling xcb_discard_reply(). This idea needs to be fleshed out so that this call is not made when get_reply() is used.

(by the way: What happens when something calls get_reply() multiple times on a cookie? I think that is not well-defined according to XCB. Put differently: Shouldn't the argument of get_reply be changed from &self to self?)

Lompik commented 5 years ago

No leak unless your program doesn't call xcb_disconnect. xcb keeps track of pending replies & frees them on disconnect.

get_reply multiple time on a cookie will just block on poll. Moving to self seems to make sense but for Drop xcb_discard_reply not so much: it won't leak & some *_unchecked don't generate a reply so xcb will generate spurious one for it!

daniel-abramov commented 5 years ago

No leak unless your program doesn't call xcb_disconnect. xcb keeps track of pending replies & frees them on disconnect.

That's too late for a releasing resources. By that time the memory might be gone. Most apps which communicate with X server actively would call xcb_disconnect() only when the app is being closed / releases the rest of the resources after accomplishing the job.

Lompik commented 5 years ago

like said in the readme rust-xcb plans to give us only an interface to xcb. A rust idiomatic probably belongs in a higher level wrapper...

but you're right, there is a problem with rust-xcb though because we can't call xcb_discard_reply on cookie and having to call get_reply is not nice.