Fix FD leak when checking lock dir permissions

NeuronRobotics / nrjavaserial

A Java Serial Port system. This is a fork of the RXTX project that uses in jar loading of the native code.

Other

344 stars 142 forks source link

Fix FD leak when checking lock dir permissions #216

Closed MrDOS closed 3 years ago

MrDOS commented 3 years ago

mkstemp(3) creates and opens a file descriptor for a temporary file, but this file descriptor was immediately discarded in favour of fopen(3)'ing the file by name and using that FILE * stream. I'm sure whoever originally wrote this code meant to use mktemp(3) instead, which only creates a unique file from a template name (equivalent to mktemp(1)).

This fixes https://github.com/openhab/openhab-core/issues/1842 and mitigates #111.

MrDOS commented 3 years ago

because the man page of mktemp(3) discourages its use:

Yeah, I saw that. I thought about leaning into the mkstemp(3) behaviour and using the FD it returns (removing the call to fopen(3) and replacing the calls to fwrite(3) and fclose(3) with write(2) and close(2)), but:

Switching (back?) to mktemp(3) is the least-invasive change.
By not using the descriptor returned by mkstemp(3), we were already subject to the same race condition bug described for mktemp(2).
I wonder if the reason this approach is preferred over access(2) in the first place is for portability to Windows; in which case, write(2) and close(2) don't exist there either.

Given that I'd like to replace all the lock file stuff with native Java implementations anyway, I'm more comfortable with this smaller change for now.

fwolter commented 3 years ago

Switching (back?) to mktemp(3) is the least-invasive change.

Fully agree.

I wonder if the reason this approach is preferred over access(2) in the first place is for portability to Windows; in which case, write(2) and close(2) don't exist there either.

🤷

Given that I'd like to replace all the lock file stuff with native Java implementations anyway

Looking forward to it!

MrDOS commented 3 years ago

I've confirmed that this does fix the issue. I wrote a small program to print the results of gnu.io.CommPortIdentifier.getPortIdentifiers() in a loop at one-second intervals, and watched the output of sudo inotifywait -m /run/lock/ and lsof -p $(pgrep -f LeakTest). With NRJavaSerial v5.2.1, I was immediately able to reproduce the leak issue; with NRJS compiled from this branch, the problem was not visible.

I'm going to try to finish up #211 then work toward cutting a new release (v5.2.2).

digitaldan commented 3 years ago

@MrDOS Thanks for all the hard work on this library! Regarding 5.2.2, any thoughts on when you might have a release? I would love to see this fix in openHAB soon! Any chance maybe to push the open issues in https://github.com/NeuronRobotics/nrjavaserial/milestone/2 to a 5.2.3 release ?