Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix leaking udev reference in LinuxUsbDevice #1678

Merged
merged 2 commits into from
Jul 7, 2021
Merged

Fix leaking udev reference in LinuxUsbDevice #1678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6247292
Refactor LinuxUsbDevice to fix leaking reference
mattmacleod Jul 7, 2021
710873c
Update changelog
mattmacleod Jul 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
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
Open in desktop
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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again, I see that it's possible to return null if the native call fails, but of the four times we call this, we only null check one of them. No need to change this PR for that but I'll have to think about whether we should update all of them.

try {
UdevEnumerate enumerate = udev.enumerateNew();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to udev_new this can theoretically return null but probably never will. Something to think about for a future fix.

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