Skip to content

Commit

Permalink
Fix "hack" from Smithay#14 by using a real udev context object
Browse files Browse the repository at this point in the history 
Summary
---
This introduces an internal (`pub(crate)`) struct `Udev`, which wraps
the libudev `struct udev*` in almost the same way that `Enumerator`
wraps `udev_enumerate*`.  This is then used by `Enumerator::new`,
`Builder::new`, and `Device::from_syspath` to ensure enumerators,
monitors, and devices always have a valid `udev` context.  Any `Device`
instances discovered via `Enumerator` or `Builder` will share the same
`Udev` instance as the `Enumerator` or `Builder` they came from, so the
overhead associated with this change should be minimal.

Tests
---
The existing unit tests did not reproduce this problem when running in
Debian 8 "jessie".  I can't explain why, although I assume that the fact
that the enumerate test limits the results to "hidraw" subsystem may be
a factor.  I added another test to enumerate all devices.

Note that the `list_devices` example program _did_ segfault on Debian
"jessie".

I've also added a test for the new `Udev` struct which exercises the
`clone`/`drop` implementation by making sure the same instance is used
each time.  I ran these tests under `valgrind` to confirm there were no
memory leaks.

Compatibility
---
I've tried to keep the compatibility issues to a minimum, however one
breaking change was unavoidable: `Enumerator`, `Builder`, and `Device`
no longer implement `FromRaw`, because their structs now maintain not
only the `struct udev_enumerate*` but also the Rust `Udev` struct which
contains the context with which that `Enumerator`/`Builder`/`Device` was
created.  This is required to keep the `struct udev*` instance alive for
the duration of other structs' lifetimes.

Ideally the `FromRaw` trait would have been crate-internal anyway, but
since it wasn't before this change I'm not attempting to change its
visibility now.  However any code that depends on `Enumerator`,
`Builder`, or `Device` implementing `FromRaw` will break with this
change, therefore this probably needs to be published to crates.io with
an incremented version number.

Closes Smithay#18
  • Loading branch information
@anelson @Drakulix
anelson authored and Drakulix committed Sep 19, 2020
1 parent 87d6e59 commit c132ed9
Show file tree
Hide file tree
Showing 5 changed files with 188 additions and 44 deletions.
55 changes: 44 additions & 11 deletions src/device.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,23 @@ use std::str::FromStr;

use libc::{c_char, dev_t};

use Udev;
use {ffi, util};

use FromRaw;
use AsRaw;

/// A structure that provides access to sysfs/kernel devices.
pub struct Device {
udev: Udev,
device: *mut ffi::udev_device,
}

impl Clone for Device {
fn clone(&self) -> Self {
unsafe { Self::from_raw(ffi::udev_device_ref(self.device)) }
Self {
udev: self.udev.clone(),
device: unsafe { ffi::udev_device_ref(self.device) },
}
}
}

Expand All @@ -31,23 +36,45 @@ impl Drop for Device {
}
}

as_ffi!(Device, device, ffi::udev_device);
as_raw!(Device, device, ffi::udev_device);

impl Device {
/// Creates a device for a given syspath.
///
/// The `syspath` parameter should be a path to the device file within the `sysfs` file system,
/// e.g., `/sys/devices/virtual/tty/tty0`.
pub fn from_syspath(syspath: &Path) -> Result<Self> {
// Create a new Udev context for this device
// It would be more efficient to allow callers to create just one context and use multiple
// devices, however that would be an API-breaking change.
//
// When devices are enumerated using an `Enumerator`, it will use `from_syspath_internal`
// which can reuse the existing `Udev` context to avoid this extra overhead.
let udev = Udev::new()?;

Self::from_syspath_internal(udev, syspath)
}

/// Creates a device for a given syspath, re-using an existing `Udev` context
///
/// The `syspath` parameter should be a path to the device file within the `sysfs` file system,
/// e.g., `/sys/devices/virtual/tty/tty0`.
pub(crate) fn from_syspath_internal(udev: Udev, syspath: &Path) -> Result<Self> {
let syspath = util::os_str_to_cstring(syspath)?;

// Hack. We use this because old version libudev check udev arg by null ptr and return error
// if udev eq nullptr. In current version first argument unused
let ptr = try_alloc!(unsafe {
ffi::udev_device_new_from_syspath([].as_mut_ptr() as *mut ffi::udev, syspath.as_ptr())
ffi::udev_device_new_from_syspath(udev.as_raw(), syspath.as_ptr())
});

Ok(unsafe { Self::from_raw(ptr) })
Ok(Self::from_raw(udev, ptr))
}

/// Creates a rust `Device` given an already created libudev `ffi::udev_device*` and a
/// corresponding `Udev` instance from which the device was created.
///
/// This guarantees that the `Udev` will live longer than the corresponding `Device`
pub(crate) fn from_raw(udev: Udev, ptr: *mut ffi::udev_device) -> Self {
Self { udev, device: ptr }
}

/// Checks whether the device has already been handled by udev.
Expand Down Expand Up @@ -106,7 +133,9 @@ impl Device {
return None;
}

Some(unsafe { Self::from_raw(ffi::udev_device_ref(ptr)) })
Some(Self::from_raw(self.udev.clone(), unsafe {
ffi::udev_device_ref(ptr)
}))
}

/// Returns the parent of the device with the matching subsystem and devtype if any.
Expand All @@ -124,14 +153,16 @@ impl Device {
return Ok(None);
}

Ok(Some(unsafe { Self::from_raw(ffi::udev_device_ref(ptr)) }))
Ok(Some(Self::from_raw(self.udev.clone(), unsafe {
ffi::udev_device_ref(ptr)
})))
}

/// Returns the parent of the device with the matching subsystem and devtype if any.
pub fn parent_with_subsystem_devtype<T: AsRef<OsStr>, U: AsRef<OsStr>>(
&self,
subsystem: T,
devtype: U
devtype: U,
) -> Result<Option<Self>> {
let subsystem = util::os_str_to_cstring(subsystem)?;
let devtype = util::os_str_to_cstring(devtype)?;
Expand All @@ -147,7 +178,9 @@ impl Device {
return Ok(None);
}

Ok(Some(unsafe { Self::from_raw(ffi::udev_device_ref(ptr)) }))
Ok(Some(Self::from_raw(self.udev.clone(), unsafe {
ffi::udev_device_ref(ptr)
})))
}

/// Returns the subsystem name of the device.
Expand Down
58 changes: 42 additions & 16 deletions src/enumerator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,27 @@ use std::ffi::OsStr;
use std::io::Result;
use std::path::Path;

use Udev;
use {ffi, util};

use {AsRaw, Device, FromRaw};
use {AsRaw, Device};

/// An enumeration context.
///
/// An Enumerator scans `/sys` for devices matching its filters. Filters are added to an Enumerator
/// by calling its `match_*` and `nomatch_*` methods. After the filters are setup, the
/// `scan_devices()` method finds devices in `/sys` that match the filters.
pub struct Enumerator {
udev: Udev,
enumerator: *mut ffi::udev_enumerate,
}

impl Clone for Enumerator {
fn clone(&self) -> Self {
unsafe { Self::from_raw(ffi::udev_enumerate_ref(self.enumerator)) }
Self {
udev: self.udev.clone(),
enumerator: unsafe { ffi::udev_enumerate_ref(self.enumerator) },
}
}
}

Expand All @@ -27,15 +32,24 @@ impl Drop for Enumerator {
}
}

as_ffi!(Enumerator, enumerator, ffi::udev_enumerate);
as_raw!(Enumerator, enumerator, ffi::udev_enumerate);

impl Enumerator {
/// Creates a new Enumerator.
pub fn new() -> Result<Self> {
// Hack. We use this because old version libudev check udev arg by null ptr and return error
// if udev eq nullptr. In current version first argument unused
let ptr = try_alloc!(unsafe { ffi::udev_enumerate_new([].as_mut_ptr() as *mut ffi::udev) });
Ok(unsafe { Self::from_raw(ptr) })
// Create a new Udev context for this enumeration
// It would be more efficient to allow callers to create just one context and use multiple
// enumerators, however that would be an API-breaking change. Since the use of Udev
// context objects isn't even enforced in more recent versions of libudev, the overhead
// associated with creating one for each enumeration is presumably low, and not worth the
// additional complexity of a breaking API change.
let udev = Udev::new()?;

let ptr = try_alloc!(unsafe { ffi::udev_enumerate_new(udev.as_raw()) });
Ok(Self {
udev,
enumerator: ptr,
})
}

/// Adds a filter that matches only initialized devices.
Expand Down Expand Up @@ -159,21 +173,18 @@ impl Enumerator {

Ok(Devices {
entry: unsafe { ffi::udev_enumerate_get_list_entry(self.enumerator) },
enumerator: unsafe { ffi::udev_enumerate_ref(self.enumerator) }
enumerator: self.clone(),
})
}
}

/// Iterator over devices.
pub struct Devices {
entry: *mut ffi::udev_list_entry,
enumerator: *mut ffi::udev_enumerate,
}

impl Drop for Devices {
fn drop(&mut self) {
unsafe { ffi::udev_enumerate_unref(self.enumerator) };
}
/// `Devices` must hold a clone of `Enumerator` to ensure the `udev_enumerate` struct (and the
/// `udev` struct which it depends on) remain allocated for the life of the `Devices` instance
enumerator: Enumerator,
}

impl Iterator for Devices {
Expand All @@ -187,7 +198,8 @@ impl Iterator for Devices {

self.entry = unsafe { ffi::udev_list_entry_get_next(self.entry) };

match Device::from_syspath(syspath) {
println!("{}", syspath.display());
match Device::from_syspath_internal(self.enumerator.udev.clone(), syspath) {
Ok(d) => return Some(d),
Err(_) => continue,
};
Expand Down Expand Up @@ -222,6 +234,20 @@ mod tests {
for dev in find_hidraws() {
println!("Found a hidraw at {:?}", dev.devnode());
}
}

// The above test which limits devices to `hidraw` did not reproduce the crash on libudev 215
// caused by the use of a bogus udev context. Clearly it's important to test all enumeration
// pathways.
//
// This test is intended to reproduce https://github.com/Smithay/udev-rs/issues/18 when run on
// a system like Debian 8 "jessie" which runs an older libudev
#[test]
fn test_enumerate_all() {
let mut en = Enumerator::new().unwrap();

for dev in en.scan_devices().unwrap() {
println!("Found a device at {:?}", dev.devnode());
}
}
}
}
13 changes: 13 additions & 0 deletions src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ extern crate mio;
pub use device::{Attribute, Attributes, Device, Properties, Property};
pub use enumerator::{Devices, Enumerator};
pub use monitor::{Builder as MonitorBuilder, Event, EventType, Socket as MonitorSocket};
pub(crate) use udev::Udev;

macro_rules! try_alloc {
($exp:expr) => {{
Expand Down Expand Up @@ -52,6 +53,13 @@ pub trait FromRaw<T: 'static> {

/// Convert from a raw pointer and the matching context
macro_rules! as_ffi {
($struct_:ident, $field:ident, $type_:ty) => {
as_raw!($struct_, $field, $type_);
from_raw!($struct_, $field, $type_);
};
}

macro_rules! as_raw {
($struct_:ident, $field:ident, $type_:ty) => {
impl $crate::AsRaw<$type_> for $struct_ {
fn as_raw(&self) -> *mut $type_ {
Expand All @@ -62,7 +70,11 @@ macro_rules! as_ffi {
self.$field
}
}
};
}

macro_rules! from_raw {
($struct_:ident, $field:ident, $type_:ty) => {
impl $crate::FromRaw<$type_> for $struct_ {
unsafe fn from_raw(t: *mut $type_) -> Self {
Self { $field: t }
Expand All @@ -74,4 +86,5 @@ macro_rules! as_ffi {
mod device;
mod enumerator;
mod monitor;
mod udev;
mod util;
42 changes: 25 additions & 17 deletions src/monitor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,30 @@ use std::os::unix::io::{AsRawFd, RawFd};
#[cfg(feature = "mio")]
use mio::{event::Evented, unix::EventedFd, Poll, PollOpt, Ready, Token};

use Udev;
use {ffi, util};

use {AsRaw, Device, FromRaw};
use {AsRaw, Device};

/// Monitors for device events.
///
/// A monitor communicates with the kernel over a socket. Filtering events is performed efficiently
/// in the kernel, and only events that match the filters are received by the socket. Filters must
/// be setup before listening for events.
pub struct Builder {
udev: Udev,
monitor: *mut ffi::udev_monitor,
}

impl Clone for Builder {
fn clone(&self) -> Self {
Self {
udev: self.udev.clone(),
monitor: unsafe { ffi::udev_monitor_ref(self.monitor) },
}
}
}

impl Drop for Builder {
fn drop(&mut self) {
unsafe {
Expand All @@ -30,20 +41,24 @@ impl Drop for Builder {
}
}

as_ffi!(Builder, monitor, ffi::udev_monitor);
as_raw!(Builder, monitor, ffi::udev_monitor);

impl Builder {
/// Creates a new `Monitor`.
pub fn new() -> Result<Self> {
// Create a new Udev context for this monitor
// It would be more efficient to allow callers to create just one context and use multiple
// monitors, however that would be an API-breaking change.
Self::with_udev(Udev::new()?)
}

/// Creates a new `Monitor` using an existing `Udev` instance
pub(crate) fn with_udev(udev: Udev) -> Result<Self> {
let name = b"udev\0".as_ptr() as *const libc::c_char;

// Hack. We use this because old version libudev check udev arg by null ptr and return error
// if udev eq nullptr. In current version first argument unused
let ptr = try_alloc!(unsafe {
ffi::udev_monitor_new_from_netlink([].as_mut_ptr() as *mut ffi::udev, name)
});
let ptr = try_alloc!(unsafe { ffi::udev_monitor_new_from_netlink(udev.as_raw(), name) });

Ok(unsafe { Self::from_raw(ptr) })
Ok(Self { udev, monitor: ptr })
}

/// Adds a filter that matches events for devices with the given subsystem.
Expand Down Expand Up @@ -113,18 +128,11 @@ impl Builder {
/// Monitors are initially setup to receive events from the kernel via a nonblocking socket. A
/// variant of `poll()` should be used on the file descriptor returned by the `AsRawFd` trait to
/// wait for new events.
#[derive(Clone)]
pub struct Socket {
inner: Builder,
}

impl Clone for Socket {
fn clone(&self) -> Self {
Self {
inner: unsafe { Builder::from_raw(ffi::udev_monitor_ref(self.inner.monitor)) },
}
}
}

impl AsRaw<ffi::udev_monitor> for Socket {
fn as_raw(&self) -> *mut ffi::udev_monitor {
self.inner.monitor
Expand Down Expand Up @@ -152,7 +160,7 @@ impl Iterator for Socket {
if ptr.is_null() {
None
} else {
let device = unsafe { Device::from_raw(ptr) };
let device = Device::from_raw(self.inner.udev.clone(), ptr);
Some(Event { device })
}
}
Expand Down
Loading

0 comments on commit c132ed9

Please sign in to comment.