SpotBugs analysis: SASL.findCookie and SASL.addCookie may fail to close streams

[ghost]

From SpotBugs:

  private String findCookie(String _context, String _id) throws IOException {
    String homedir = System.getProperty("user.home");
    File f = new File(homedir + "/.dbus-keyrings/" + _context);
    BufferedReader r = new BufferedReader(new InputStreamReader(new FileInputStream(f)));
    String s = null;
    String lCookie = null;
    long now = System.currentTimeMillis() / 1000;
    while (null != (s = r.readLine())) {

There seem to be a few issues with this code, in order of importance:

• If r.readLine() throws an exception, then the readers will not be closed, which will leak file descriptors. Recommend using try-with-resources to ensure that the readers are closed.
• Using System.currentTimeMillis() isn't monotonic, so comparisons made against previous invocations aren't necessarily guaranteed to be linear in time.
• Consider File f = Paths.get(homedir, ".dbus-keyrings", _convert).toFile();. See https://stackoverflow.com/a/412495/59087
• As a trivial optimization, I typically cache System.getProperty(...) in a static constant to avoid calling getProperty(...) every time.

It looks like a constant for the keyrings directory would avoid some trivial duplication:

private static final String USER_HOME = System.getProperty( "user.home" );
private static final Path KEYRINGS = Paths.get( USER_HOME, ".dbus-keyrings" );

That would help reduce some boilerplate in addCookie(...):

File cookiefile = KEYRINGS.resolve(_context).toFile();
File lock = KEYRINGS.resolve(_context + ".lock").toFile();
File temp = KEYRINGS.resolve(_context + ".temp").toFile();

From SpotBugs:

It looks like there are two potential leaks:

BufferedReader r = new BufferedReader(new InputStreamReader(new FileInputStream(cookiefile)));
// ...
PrintWriter w = new PrintWriter(new FileOutputStream(temp));

I believe both need to use try-with-resources to ensure correctness.

In my recent past, there was a C bug that caused an application to block indefinitely because the clock being used wasn't monotonic. When the date changed due to an NTP server update, the time shifted by two days, blocking the semaphore. We replaced the system clock call with a monotonic timer to resolve the issue. Does that exact same problem lurk here?

        // acquire lock
        long start = System.currentTimeMillis();

        while (!lock.createNewFile() && LOCK_TIMEOUT > (System.currentTimeMillis() - start)) { //NOPMD
        }

If the NTP server kicks in, the loop could take much longer than LOCK_TIMEOUT to expire, resulting in an effective hang.