Fix FD leak when checking lock dir permissions

[MrDOS]

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 openhab/openhab-core#1842 and mitigates #111.

[fwolter]

Nice finding! I second your theory. Maybe it used to be mktemp(3) and somebody replaced it by mkstemp(3) without thinking much about it, because the man page of mktemp(3) discourages its use:

Never use mktemp(). Some implementations follow 4.3BSD and replace XXXXXX by the current process ID and a single letter, so that at most 26 different names can be returned. Since on the one hand the names are easy to guess, and on the other hand there is a race between testing whether the name exists and opening the file, every use of mktemp() is a security risk. The race is avoided by mkstemp(3).

I think this limitation is okay for us...

[MrDOS]

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]

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]

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]

@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 ?