MadLittleMods / node-usb-detection

List USB devices in system and detect changes on them.
MIT License
371 stars 114 forks source link

Fixed a crash after stopMonitoring on Linux in Electron #138

Closed antelle closed 2 years ago

antelle commented 3 years ago

Hi! 👋

Thanks for the library, it's awesome and seems to be much more stable compared to alternatives! 🎉

The library works like this (simplified pseuducode):

start() {
    isRunning = true;
    fd = udev_monitor_get_fd();
    start_polling();
}
stop() {
    isRunning = false;
    destroy_udev();  <-- destroys fd
}
poll() {
    while (isRunning) {
        poll(fd, 100ms);
    }
}

When we call stop, the device fd is destroyed because we destroy udev handle, however if we're still using this fd in poll, it causes SIGSEGV in Electron. It doesn't repeat in node.js, no idea why.

Simple PoC:

const { app, BrowserWindow } = require('electron');

app.whenReady().then(() => {
    const win = new BrowserWindow({ width: 800, height: 600 });
    win.loadURL('https://example.com');

    const mod = require('.');

    mod.startMonitoring();
    console.log('started');
    setTimeout(() => {
        mod.stopMonitoring();
        console.log('stopped');
        app.quit();
    }, 1000);
});

Compile it with:

npm i electron-rebuild
node_modules/.bin/electron-rebuild --version 12.0.0 --only .
node_modules/.bin/electron electron-test.js

Result (not always, repeat it several times):

started
stopped
~/node-usb-detection/node_modules/electron/dist/electron exited with signal SIGSEGV

This change moves destroying of the library handle to the callback that's called after "work" is complete.

While this is not critical because stopMonitoring must be called before exit, it's better if the app quits correctly instead of crashing.

I've tested only a simple case and I assume, this needs a bit more extensive testing.

dfischer23 commented 2 years ago

when i use stopMonitoring() from within plain nodejs (17.1.0) on my Arch linux (systemd), i get: Assertion 'm' failed at src/libsystemd/sd-device/device-monitor.c:443, function device_monitor_receive_device(). Aborting.

This PR fixes it, allowing me to cleanly quit my program. Please merge and release!

MadLittleMods commented 2 years ago

Closing as https://github.com/MadLittleMods/node-usb-detection/pull/162 has merged these changes 🚀

Everyone mentioned in the upcoming 4.14.0 changelog and will be released once I figure out why the GitHub Actions for the Windows builds aren't working, https://github.com/MadLittleMods/node-usb-detection/issues/164

Thanks for the contribution @antelle @umbernhard @Julusian! 🐙