
Description
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 callinggetProperty(...)
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.
Activity