Repository: hannobraun/inotify-rs (language: rust)

Issue:
`Eq` implementation of `WatchDescriptor` is wrong
From [Stack Overflow](https://stackoverflow.com/questions/45315521/how-do-i-use-the-watchdescriptor-struct-generated-by-inotify-rs-to-match-events/45316225#45316225):

> As noted above, inotify watch descriptor values are scoped per parent inotify file descriptor. This makes the use of [derive(Eq)] on WatchDescriptor highly questionable. Looks like a straight-up bug.

I haven't had the chance to look into the details myself so far, but this seems legit.


Discussion / hints:
I have tested this by creating multiple `Inotify` instances and matching the `WatchDescriptors` they generate.

`assert_eq!(wd_from_inotify1, wd_from_inotify2);` passes when the contained `c_int` is the same for both `WatchDescriptors`s. This should not be the case.

While the `Eq` trait does seem useful within the scope of an `Inotify` instance, the potential to match across instances can break things.

I don't think removing the `Eq` trait is the right way to address this issue. Perhaps it makes more sense for the `Inotify` struct to generate an identifier unique to itself. This can be passed to generated `WatchDescriptor`s as a field. This would retain the usability of the `Eq` trait within the scope of the parent `Inotify`, and protect against matching `WatchDescriptor`s between instances of `Inotify`
I think the most straight-forward way to address this would be to add the inotify instance's file descriptor to `WatchDescriptor`. That would do nothing to solve #61, but at least the `Eq` implementation would be working correctly.
Yes, that does seem to be the most practical way of fixing the `Eq` implementation. I've implemented a field of type `RawFd` in the `WatchDescriptor` struct in my working tree.

I'll do a PR once I've figured out what can be done about #61. My tree doesn't build at the moment. 
If you make a pull request for adding the `RawFd`, I'd definitely accept it. Yes, eventually we want a complete solution that also solves #61, but I suspect that will require more thinking and therefore more time. Until then, fixing the `Eq` issue in a simple way will be very welcome.
Adding the `RawFd` field to `WatchDescriptor` introduces another issue: Inotify[1] itself doesn't associate watch descriptors and events with a particular Inotify instance.

From the Inotify source (accessed from [here](http://elixir.free-electrons.com/linux/latest/source/fs/notify/inotify/inotify_user.c#L189)):

	pad_name_len = round_event_name_len(fsn_event);
	inotify_event.len = pad_name_len;
	inotify_event.mask = inotify_mask_to_arg(fsn_event->mask);
	inotify_event.wd = event->wd;
	inotify_event.cookie = event->sync_cookie;

Implementing a mechanism to prevent `WatchDescriptors` from being shared between `Inotify`[2] instances (by adding a `RawFd` field to the `WatchDescriptor` struct) is possible, but it requires changes to inotify-rs such that the `Inotify` and `Event` structs will need to handle checking `WatchDescriptor` instances for `RawFd`. For instance, the `add_watch` method would then need to return a `WatchDescriptor` with a RawFd field assigned `self.0`, 

    match wd {
        -1 => Err(io::Error::last_os_error()),
        _  => Ok(WatchDescriptor(wd, self.0)),
    }

and the `rm_watch` method would need to check this against the RawFd field of the parent `Inotify` instance. This adds extraneous functionality to the library over and above the scope of the original Inotify library, since Inotify doesn't include information about the originating Inotify instance with watch descriptors.

This is a major change that affects how the library handles events and watch descriptors, and would also be a breaking change.

[1]Inotify denotes the inotify.c library
[2]`Inotify` denotes the Inotify struct in inotify-rs and is formatted as inline code to make it distinct from Inotify above
> This is a major change that affects how the library handles events and watch descriptors, and would also be a breaking change.

I think the additional overhead on top of inotify is warranted in this case. I believe it is reasonable to expect an idiomatic Rust wrapper to prevent misuse of the underlying API, as far as that is practical.

Breaking changes are acceptable at this stage. That's why the library is still at an 0.x version.
If there’s going to be breaking changes, why not use the opportunity to provide an easier and more manageable interface to the user?

I think making use of a hashmap for indexing watch descriptors is a good way forward:

The `add_watch()` method creates a HashMap<key, watchdescriptor> and returns the key.

The `rm_watch()` method takes this key and looks it up in the HashMap.

Events are also returned with the same key to link them to HashMaps.

The keys could be unique (how?) to prevent contamination between instances of `Inotify`. Alternatively, the HashMap itself could be passed to the user as Inotify.HashMap.

I’m just thinking out loud right now. I’m going to try something and push a branch if it seems to work.
I'm don't understand what the benefit of this would be. Why give the user a key to refer to the `WatchDescriptor` instead of just giving them the `WatchDescriptor` directly? What is the purpose of the `HashMap`?
I'm sorry about that momentary lapse of reason. Turns out I was doing something very wrong.

I'm making a PR for this bugfix.
I believe this should be closed.
I think this should stay open. While #67 addressed the main defect, I thought of another case where the `Eq` implementation could be wrong: If the inotify file descriptor is being reused, and an old `WatchDescriptor` is still around.

Usually, this shouldn't be a problem. You can just create one `Inotify` instance and use that forever. However, people might use the library differently (see #68, where a user talks about one `Inotify` instance per watched directory). A long-running process that watches many directories and opens/closes many files might run into this bug sooner or later.

I have an idea on how to address this (warning, pure idea, not tested in any way): Convert the `fd` in `Inotify` from a `RawFd` to a [`Rc<RawFd>`](https://doc.rust-lang.org/std/rc/struct.Rc.html). Convert `fd` in `WatchDescriptor` to a [`Weak<RawFd>`](https://doc.rust-lang.org/std/rc/struct.Weak.html) that points to the same `RawFd`. Methods that handle `WatchDescriptor` would still need to compare the two `RawFd`s, however, by accessing the `WatchDescriptors`'s `RawFd` through the `Weak` pointer, they would make sure the original `RawFd` was never dropped.

I think this would also solve other issues mentioned in #61. However, it would come with additional cost, most notably one heap allocation per `Inotify` instance. This sounds acceptable to me, but maybe we should ask the [notify](https://crates.io/crates/notify) developers for feedback before merging anything like this. On the other hand, no one ever complained about the heap-allocated buffer in `Inotify` when that was still a thing.
I did a bit of research. It seems that Linux immediately re-assigns the numbers from closed file descriptors. I've tested with this piece of code:
```rust
let inotify_1 = Inotify::init().unwrap();
print!("{}\n", inotify_1.as_raw_fd());
inotify_1.close().unwrap();
let inotify_2 = Inotify::init().unwrap();
print!("{}\n", inotify_2.as_raw_fd());
```
It prints the same number twice.

This makes it somewhat likely to run into this issue, when handling multiple `Inotify` instances. Knowing that, I think the overhead of the solution I outlined above is clearly worth it (altough I still haven't looked at the solution any closer, so I don't know if it will work out).

Fix patch:
diff --git a/inotify/src/lib.rs b/inotify/src/lib.rs
index 4e0a675..59b1915 100644
--- a/inotify/src/lib.rs
+++ b/inotify/src/lib.rs
@@ -29,6 +29,10 @@ extern crate inotify_sys as ffi;
 
 
 use std::mem;
+use std::hash::{
+    Hash,
+    Hasher,
+};
 use std::io;
 use std::io::ErrorKind;
 use std::os::unix::ffi::OsStrExt;
@@ -39,6 +43,10 @@ use std::os::unix::io::{
     RawFd,
 };
 use std::path::Path;
+use std::rc::{
+    Rc,
+    Weak,
+};
 use std::slice;
 use std::ffi::{
     OsStr,
@@ -104,7 +112,10 @@ use libc::{
 ///     // Handle event
 /// }
 /// ```
-pub struct Inotify(RawFd);
+pub struct Inotify {
+    fd           : Rc<RawFd>,
+    close_on_drop: bool,
+}
 
 impl Inotify {
     /// Creates an [`Inotify`] instance
@@ -159,7 +170,11 @@ impl Inotify {
 
         match fd {
             -1 => Err(io::Error::last_os_error()),
-            _  => Ok(Inotify(fd)),
+            _  =>
+                Ok(Inotify {
+                    fd           : Rc::new(fd),
+                    close_on_drop: true,
+                }),
         }
     }
 
@@ -233,7 +248,7 @@ impl Inotify {
 
         let wd = unsafe {
             ffi::inotify_add_watch(
-                self.0,
+                *self.fd,
                 path.as_ptr() as *const _,
                 mask.bits(),
             )
@@ -241,7 +256,7 @@ impl Inotify {
 
         match wd {
             -1 => Err(io::Error::last_os_error()),
-            _  => Ok(WatchDescriptor{id: wd, fd: self.0}),
+            _  => Ok(WatchDescriptor{ id: wd, fd: Rc::downgrade(&self.fd) }),
         }
     }
 
@@ -294,10 +309,14 @@ impl Inotify {
     /// [`Inotify::add_watch`]: struct.Inotify.html#method.add_watch
     /// [`Event`]: struct.Event.html
     pub fn rm_watch(&mut self, wd: WatchDescriptor) -> io::Result<()> {
-        if self.0 != wd.fd {
-            return Err(io::Error::new(ErrorKind::InvalidInput, "Invalid WatchDescriptor"))
+        if wd.fd.upgrade().as_ref() != Some(&self.fd) {
+            return Err(io::Error::new(
+                ErrorKind::InvalidInput,
+                "Invalid WatchDescriptor",
+            ));
         }
-        let result = unsafe { ffi::inotify_rm_watch(self.0, wd.id) }; 
+
+        let result = unsafe { ffi::inotify_rm_watch(*self.fd, wd.id) };
         match result {
             0  => Ok(()),
             -1 => Err(io::Error::last_os_error()),
@@ -322,14 +341,12 @@ impl Inotify {
     pub fn read_events_blocking<'a>(&mut self, buffer: &'a mut [u8])
         -> io::Result<Events<'a>>
     {
-        let fd = self.0;
-
         unsafe {
-            fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) & !O_NONBLOCK)
+            fcntl(*self.fd, F_SETFL, fcntl(*self.fd, F_GETFL) & !O_NONBLOCK)
         };
         let result = self.read_events(buffer);
         unsafe {
-            fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) | O_NONBLOCK)
+            fcntl(*self.fd, F_SETFL, fcntl(*self.fd, F_GETFL) | O_NONBLOCK)
         };
 
         result
@@ -381,7 +398,7 @@ impl Inotify {
     {
         let num_bytes = unsafe {
             ffi::read(
-                self.0,
+                *self.fd,
                 buffer.as_mut_ptr() as *mut c_void,
                 buffer.len() as size_t
             )
@@ -399,7 +416,7 @@ impl Inotify {
             -1 => {
                 let error = io::Error::last_os_error();
                 if error.kind() == io::ErrorKind::WouldBlock {
-                    return Ok(Events::new(self.0, buffer, 0));
+                    return Ok(Events::new(Rc::downgrade(&self.fd), buffer, 0));
                 }
                 else {
                     return Err(error);
@@ -429,7 +446,7 @@ impl Inotify {
             }
         };
 
-        Ok(Events::new(self.0, buffer, num_bytes))
+        Ok(Events::new(Rc::downgrade(&self.fd), buffer, num_bytes))
     }
 
     /// Closes the inotify instance
@@ -457,8 +474,13 @@ impl Inotify {
     ///
     /// [`Inotify`]: struct.Inotify.html
     /// [`close`]: ../libc/fn.close.html
-    pub fn close(self) -> io::Result<()> {
-        match unsafe { ffi::close(self.0) } {
+    pub fn close(mut self) -> io::Result<()> {
+        // `self` will be dropped when this method returns. The `Drop`
+        // implementation will attempt to close the file descriptor again,
+        // unless this flag here is cleared.
+        self.close_on_drop = false;
+
+        match unsafe { ffi::close(*self.fd) } {
             0 => Ok(()),
             _ => Err(io::Error::last_os_error()),
         }
@@ -467,29 +489,31 @@ impl Inotify {
 
 impl Drop for Inotify {
     fn drop(&mut self) {
-        if self.0 != -1 {
-            unsafe { ffi::close(self.0); }
+        if self.close_on_drop {
+            unsafe { ffi::close(*self.fd); }
         }
     }
 }
 
 impl AsRawFd for Inotify {
     fn as_raw_fd(&self) -> RawFd {
-        self.0
+        *self.fd
     }
 }
 
 impl FromRawFd for Inotify {
     unsafe fn from_raw_fd(fd: RawFd) -> Self {
-        Inotify(fd)
+        Inotify {
+            fd           : Rc::new(fd),
+            close_on_drop: true,
+        }
     }
 }
 
 impl IntoRawFd for Inotify {
-    fn into_raw_fd(self) -> RawFd {
-        let fd = self.0;
-        mem::forget(self);
-        fd
+    fn into_raw_fd(mut self) -> RawFd {
+        self.close_on_drop = false;
+        *self.fd
     }
 }
 
@@ -592,12 +616,37 @@ pub use self::watch_mask::WatchMask;
 /// [`Inotify::add_watch`]: struct.Inotify.html#method.add_watch
 /// [`Inotify::rm_watch`]: struct.Inotify.html#method.rm_watch
 /// [`Event`]: struct.Event.html
-#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)]
+#[derive(Clone, Debug)]
 pub struct WatchDescriptor{
     id: c_int,
-    fd: RawFd,
+    fd: Weak<RawFd>,
+}
+
+impl Eq for WatchDescriptor {}
+
+impl PartialEq for WatchDescriptor {
+    fn eq(&self, other: &Self) -> bool {
+        let self_fd  = self.fd.upgrade();
+        let other_fd = other.fd.upgrade();
+
+        self.id == other.id && self_fd.is_some() && self_fd == other_fd
+    }
 }
 
+impl Hash for WatchDescriptor {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        // This function only takes `self.id` into account, as `self.fd` is a
+        // weak pointer that might no longer be available. Since neither
+        // panicking nor changing the hash depending on whether it's available
+        // is acceptable, we just don't look at it at all.
+        // I don't think that this influences storage in a `HashMap` or
+        // `HashSet` negatively, as storing `WatchDescriptor`s from different
+        // `Inotify` instances seems like something of an antipattern anyway.
+        self.id.hash(state);
+    }
+}
+
+
 /// Iterates over inotify events
 ///
 /// Iterates over the events returned by [`Inotify::read_events_blocking`] or
@@ -606,14 +655,14 @@ pub struct WatchDescriptor{
 /// [`Inotify::read_events_blocking`]: struct.Inotify.html#method.read_events_blocking
 /// [`Inotify::read_events`]: struct.Inotify.html#method.read_events
 pub struct Events<'a> {
-    fd       : RawFd,
+    fd       : Weak<RawFd>,
     buffer   : &'a [u8],
     num_bytes: usize,
     pos      : usize,
 }
 
 impl<'a> Events<'a> {
-    fn new(fd: RawFd, buffer: &'a [u8], num_bytes: usize) -> Self {
+    fn new(fd: Weak<RawFd>, buffer: &'a [u8], num_bytes: usize) -> Self {
         Events {
             fd       : fd,
             buffer   : buffer,
@@ -663,7 +712,7 @@ impl<'a> Iterator for Events<'a> {
             self.pos += event_size + event.len as usize;
 
             Some(Event::new(
-                self.fd,
+                self.fd.clone(),
                 &event,
                 OsStr::from_bytes(name),
             ))
@@ -729,7 +778,9 @@ pub struct Event<'a> {
 }
 
 impl<'a> Event<'a> {
-    fn new(fd: RawFd, event: &ffi::inotify_event, name: &'a OsStr) -> Self {
+    fn new(fd: Weak<RawFd>, event: &ffi::inotify_event, name: &'a OsStr)
+        -> Self
+    {
         let mask = EventMask::from_bits(event.mask)
             .expect("Failed to convert event mask. This indicates a bug.");
 


Test patch:
diff --git a/inotify/tests/main.rs b/inotify/tests/main.rs
index cdd8932..05fb761 100644
--- a/inotify/tests/main.rs
+++ b/inotify/tests/main.rs
@@ -16,6 +16,11 @@ use std::io::{
     Write,
     ErrorKind,
 };
+use std::os::unix::io::{
+    AsRawFd,
+    FromRawFd,
+    IntoRawFd,
+};
 use std::path::PathBuf;
 use tempdir::TempDir;
 
@@ -97,23 +102,90 @@ fn it_should_not_accept_watchdescriptors_from_other_instances() {
     let (path, _) = testdir.new_file();
 
     let mut inotify = Inotify::init().unwrap();
-    let wd1 = inotify.add_watch(&path, watch_mask::ACCESS).unwrap();
+    let _ = inotify.add_watch(&path, watch_mask::ACCESS).unwrap();
 
     let mut second_inotify = Inotify::init().unwrap();
     let wd2 = second_inotify.add_watch(&path, watch_mask::ACCESS).unwrap();
 
-    assert!(wd1 != wd2);
     assert_eq!(inotify.rm_watch(wd2).unwrap_err().kind(), ErrorKind::InvalidInput);
 }
 
 #[test]
-fn it_should_implement_raw_fd_traits_correctly() {
-    use std::os::unix::io::{
-        FromRawFd,
-        IntoRawFd,
+fn watch_descriptors_from_different_inotify_instances_should_not_be_equal() {
+    let mut testdir = TestDir::new();
+    let (path, _) = testdir.new_file();
+
+    let mut inotify_1 = Inotify::init()
+        .unwrap();
+    let mut inotify_2 = Inotify::init()
+        .unwrap();
+
+    let wd_1 = inotify_1
+        .add_watch(&path, watch_mask::ACCESS)
+        .unwrap();
+    let wd_2 = inotify_2
+        .add_watch(&path, watch_mask::ACCESS)
+        .unwrap();
+
+    // As far as inotify is concerned, watch descriptors are just integers that
+    // are scoped per inotify instance. This means that multiple instances will
+    // produce the same watch descriptor number, a case we want inotify-rs to
+    // detect.
+    assert!(wd_1 != wd_2);
+}
+
+#[test]
+fn watch_descriptor_equality_should_not_be_confused_by_reused_fds() {
+    let mut testdir = TestDir::new();
+    let (path, _) = testdir.new_file();
+
+    // When a new inotify instance is created directly after closing another
+    // one, it is possible that the file descriptor is reused immediately, and
+    // we end up with a new instance that has the same file descriptor as the
+    // old one.
+    // This is quite likely, but it doesn't happen every time. Therefore we may
+    // need a few tries until we find two instances where that is the case.
+    let (wd_1, mut inotify_2) = loop {
+        let mut inotify_1 = Inotify::init()
+            .unwrap();
+
+        let wd_1 = inotify_1
+            .add_watch(&path, watch_mask::ACCESS)
+            .unwrap();
+        let fd_1 = inotify_1.as_raw_fd();
+
+        inotify_1
+            .close()
+            .unwrap();
+        let inotify_2 = Inotify::init()
+            .unwrap();
+
+        if fd_1 == inotify_2.as_raw_fd() {
+            break (wd_1, inotify_2);
+        }
     };
 
+    let wd_2 = inotify_2
+        .add_watch(&path, watch_mask::ACCESS)
+        .unwrap();
+
+    // The way we engineered this situation, both `WatchDescriptor` instances
+    // have the same fields. They still come from different inotify instances
+    // though, so they shouldn't be equal.
+    assert!(wd_1 != wd_2);
+
+    inotify_2
+        .close()
+        .unwrap();
 
+    // A little extra gotcha: If both inotify instances are closed, and the `Eq`
+    // implementation naively compares the weak pointers, both will be `None`,
+    // making them equal. Let's make sure this isn't the case.
+    assert!(wd_1 != wd_2);
+}
+
+#[test]
+fn it_should_implement_raw_fd_traits_correctly() {
     let fd = Inotify::init()
         .expect("Failed to initialize inotify instance")
         .into_raw_fd();