Skip to content

Commit

Permalink
Fix leaking udev reference in LinuxUsbDevice (#1678)
Browse files Browse the repository at this point in the history 
* Refactor LinuxUsbDevice to fix leaking reference

This change fixes a leaking udev device reference when enumerating
devices. Devices with a devtype that was not "usb_device" were never
freed during the enumeration.

The enumeration process has been refactored (in line with the
implementation in LinuxHWDiskStore) to use the more object-oriented
API for udev that's now available from JNA.

* Update changelog
  • Loading branch information
@mattmacleod
mattmacleod committed Jul 7, 2021
1 parent 47ccbfb commit d6e49c4
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 53 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

##### Bug fixes / Improvements
* [#1673](https://github.com/oshi/oshi/pull/1673): Fix FreeBSD ps command arguments for context switches. [@basil](https://github.com/basil)
* [#1678](https://github.com/oshi/oshi/pull/1678): Refactor to fix leaking udev reference in LinuxUsbDevice - [@mattmacleod](https://github.com/mattmacleod)

# 5.7.0 (2021-04-01), 5.7.1 (2021-04-15), 5.7.2 (2021-05-01), 5.7.3 (2021-05-16), 5.7.4 (2021-05-30), 5.7.5 (2021-06-12)

Expand Down
117 changes: 64 additions & 53 deletions oshi-core/src/main/java/oshi/hardware/platform/linux/LinuxUsbDevice.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,14 @@
@Immutable
public class LinuxUsbDevice extends AbstractUsbDevice {

private static final String SUBSYSTEM_USB = "usb";
private static final String DEVTYPE_USB_DEVICE = "usb_device";
private static final String ATTR_PRODUCT = "product";
private static final String ATTR_MANUFACTURER = "manufacturer";
private static final String ATTR_VENDOR_ID = "idVendor";
private static final String ATTR_PRODUCT_ID = "idProduct";
private static final String ATTR_SERIAL = "serial";

public LinuxUsbDevice(String name, String vendor, String vendorId, String productId, String serialNumber,
String uniqueDeviceId, List<UsbDevice> connectedDevices) {
super(name, vendor, vendorId, productId, serialNumber, uniqueDeviceId, connectedDevices);
Expand Down Expand Up @@ -82,73 +90,76 @@ public static List<UsbDevice> getUsbDevices(boolean tree) {
}

private static List<UsbDevice> getUsbDevices() {
// Enumerate all usb devices and build information maps
Udev.UdevContext udev = Udev.INSTANCE.udev_new();
// Create a list of the devices in the 'usb' subsystem.
UdevEnumerate enumerate = Udev.INSTANCE.udev_enumerate_new(udev);
Udev.INSTANCE.udev_enumerate_add_match_subsystem(enumerate, "usb");
Udev.INSTANCE.udev_enumerate_scan_devices(enumerate);
UdevListEntry devices = Udev.INSTANCE.udev_enumerate_get_list_entry(enumerate);

// Build a list of devices with no parent; these will be the roots
List<String> usbControllers = new ArrayList<>();

// Maps to store information using device node path as the key
// Maps to store information using device syspath as the key
Map<String, String> nameMap = new HashMap<>();
Map<String, String> vendorMap = new HashMap<>();
Map<String, String> vendorIdMap = new HashMap<>();
Map<String, String> productIdMap = new HashMap<>();
Map<String, String> serialMap = new HashMap<>();
Map<String, List<String>> hubMap = new HashMap<>();

// For each item enumerated, store information in the maps
for (UdevListEntry dev_list_entry = devices; dev_list_entry != null; dev_list_entry = Udev.INSTANCE
.udev_list_entry_get_next(dev_list_entry)) {
// Enumerate all usb devices and build information maps
Udev.UdevContext udev = Udev.INSTANCE.udev_new();
try {
UdevEnumerate enumerate = udev.enumerateNew();
try {
enumerate.addMatchSubsystem(SUBSYSTEM_USB);
enumerate.scanDevices();

// Get the filename of the /sys entry for the device and create a
// udev_device object (dev) representing it
String path = Udev.INSTANCE.udev_list_entry_get_name(dev_list_entry);
UdevDevice dev = Udev.INSTANCE.udev_device_new_from_syspath(udev, path);
// Ignore interfaces
if (!"usb_device".equals(Udev.INSTANCE.udev_device_get_devtype(dev))) {
continue;
}
// For each item enumerated, store information in the maps
for (UdevListEntry entry = enumerate.getListEntry(); entry != null; entry = entry.getNext()) {
String syspath = entry.getName();
UdevDevice device = udev.deviceNewFromSyspath(syspath);
if (device != null) {
try {
// Only include usb_device devtype, skipping usb_interface
if (DEVTYPE_USB_DEVICE.equals(device.getDevtype())) {
String value = device.getSysattrValue(ATTR_PRODUCT);
if (value != null) {
nameMap.put(syspath, value);
}
value = device.getSysattrValue(ATTR_MANUFACTURER);
if (value != null) {
vendorMap.put(syspath, value);
}
value = device.getSysattrValue(ATTR_VENDOR_ID);
if (value != null) {
vendorIdMap.put(syspath, value);
}
value = device.getSysattrValue(ATTR_PRODUCT_ID);
if (value != null) {
productIdMap.put(syspath, value);
}
value = device.getSysattrValue(ATTR_SERIAL);
if (value != null) {
serialMap.put(syspath, value);
}

// Use the path as the key for the maps
String value = Udev.INSTANCE.udev_device_get_sysattr_value(dev, "product");
if (value != null) {
nameMap.put(path, value);
}
value = Udev.INSTANCE.udev_device_get_sysattr_value(dev, "manufacturer");
if (value != null) {
vendorMap.put(path, value);
}
value = Udev.INSTANCE.udev_device_get_sysattr_value(dev, "idVendor");
if (value != null) {
vendorIdMap.put(path, value);
}
value = Udev.INSTANCE.udev_device_get_sysattr_value(dev, "idProduct");
if (value != null) {
productIdMap.put(path, value);
}
value = Udev.INSTANCE.udev_device_get_sysattr_value(dev, "serial");
if (value != null) {
serialMap.put(path, value);
}
UdevDevice parent = Udev.INSTANCE.udev_device_get_parent_with_subsystem_devtype(dev, "usb", "usb_device");
if (parent == null) {
// This is a controller with no parent, add to list
usbControllers.add(path);
} else {
// Add child path (path variable) to parent's path
String parentPath = Udev.INSTANCE.udev_device_get_syspath(parent);
hubMap.computeIfAbsent(parentPath, x -> new ArrayList<>()).add(path);
UdevDevice parent = device.getParentWithSubsystemDevtype(SUBSYSTEM_USB,
DEVTYPE_USB_DEVICE);
if (parent == null) {
// This is a controller with no parent, add to list
usbControllers.add(syspath);
} else {
// Add child syspath to parent's path
String parentPath = parent.getSyspath();
hubMap.computeIfAbsent(parentPath, x -> new ArrayList<>()).add(syspath);
}
}
} finally {
device.unref();
}
}
}
} finally {
enumerate.unref();
}
Udev.INSTANCE.udev_device_unref(dev);
} finally {
udev.unref();
}
// Free the enumerator object
Udev.INSTANCE.udev_enumerate_unref(enumerate);
Udev.INSTANCE.udev_unref(udev);

// Build tree and return
List<UsbDevice> controllerDevices = new ArrayList<>();
Expand Down

0 comments on commit d6e49c4

Please sign in to comment.