Smithay / calloop

A callback-based Event Loop
MIT License
177 stars 35 forks source link

Prevent dangling pointers being left in the system's polling mechanim and reduce the amount of unsafe code in the library #76

Closed detly closed 2 years ago

detly commented 2 years ago

Previously, dropping a source without unregistering it risked leaving a dangling pointer to token memory in the underlying polling mechanism. Now, the Poller type is responsible for keeping track of what data the system's poller (eg. epoll) is still pointing to, and maintaining ownership of it.

This commit also slightly reduces the amount of unsafe code related to pointer dereferences and adds some notes justifying the safety of these operations.

Notes not in the commit message

The "reduce amount of unsafe code" comment simply means I took a couple of things like:

unsafe { *(event.data() as usize as *const Token) }

...and moved the part that can be done without unsafe out. It's not a big deal, but IME this means you're less likely to accidentally put another unsafe operation in there without noticing. I also added some comments so that anyone who wants to eg. port or make changes has more information.

Token changes

How committed were you to the "invalid Token" mechanic? Because I replaced that with Option<Token>. I know the comment said that it's to prevent having Option<Token> all over the place, but with the recent changes (slotmap + token factory), there's really only a couple of places it occurs now. I can revert only those changes if you like, they're not essential, they just helped while I was hacking and I left them in.

Leak amplification and panics

If an event source is dropped before being unregistered, the Token and file descriptor won't be removed from the map in Poller. This means the Token memory will never be deallocated. More severely, if the file descriptor is re-used, this will trigger the panic here. It's debatable whether this needs to be so strict.

New dependency

There's a new dependency on vec_map.

Size changes

My changes seem to add about 5kB to the release + stripped rlib on Linux. I'll see if I can pare that down at all.

Fixes #74.

codecov[bot] commented 2 years ago

Codecov Report

Merging #76 (3054607) into master (e082cc1) will decrease coverage by 0.42%. The diff coverage is 87.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
- Coverage   82.00%   81.57%   -0.43%     
==========================================
  Files          13       15       +2     
  Lines         950      988      +38     
==========================================
+ Hits          779      806      +27     
- Misses        171      182      +11     
Impacted Files Coverage Δ
src/io.rs 60.62% <76.47%> (-1.16%) :arrow_down:
src/sys/mod.rs 87.75% <85.00%> (-3.92%) :arrow_down:
src/sources/generic.rs 83.33% <100.00%> (+3.92%) :arrow_up:
src/sys/epoll.rs 97.36% <100.00%> (+0.14%) :arrow_up:
src/sys/kqueue.rs 98.29% <100.00%> (+0.06%) :arrow_up:
src/sources/timer.rs
src/sources/timer/threaded.rs 0.00% <0.00%> (ø)
src/sources/timer/mod.rs 85.52% <0.00%> (ø)
src/sources/timer/timerfd.rs 93.54% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e082cc1...3054607. Read the comment docs.

detly commented 2 years ago

My changes seem to add about 5kB to the release + stripped rlib on Linux. I'll see if I can pare that down at all.

I couldn't figure this out. I had assumed it was due to the extra code in src/sys/mod.rs, but looking at the assembly it's not. If it's important to you, I can keep looking, otherwise I won't bother. I can tell you that on all of my actual binary projects, it hasn't affected binary size.

On another note, I don't understand why the FreeBSD tests are failing - is it the final grcov pass?

grcov . --binary-path ./target/debug -s . -t lcov --branch --llvm --ignore-not-existing --keep-only "src/sys/*" --excl-br-start "mod tests \{" --excl-start "mod tests \{" --excl-br-line "#\[derive\(" --excl-line "#\[derive\(" -o lcov.info
06:38:55 �[0m�[31m[ERROR] �[0mA panic occurred at src/producer.rs:544: No input files found
elinorbgr commented 2 years ago

Alright, I like this, thanks a lot! :+1: