Bug 51704 - Dubious use of mkdirs() return code in juli FileHandler
Summary: Dubious use of mkdirs() return code in juli FileHandler
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Catalina (show other bugs)
Version: trunk
Hardware: PC Windows XP
: P2 normal (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-22 13:17 UTC by Sebb
Modified: 2011-08-26 17:12 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sebb 2011-08-22 13:17:27 UTC
File#mkdirs() only returns true if the method created the directory itself.

If mkdirs() returns false, it is still possible for the directory to exist.

Thus the code in FileHandler at [1], i.e.


364     // Create the directory if necessary
365 	File dir = new File(directory);
366 	if (!dir.exists() && !dir.mkdirs()) {
367 	    reportError("Unable to create [" + dir + "]", null,
368 	    ErrorManager.OPEN_FAILURE);
369 	    writer = null;
370 	    return;
371 	}

can generate an error even though the directory now exists.

It would be safer to code the check as follows:

366 	if (!dir.mkdirs() && !dir.exists()) {

There is no need to call dir.exists() before mkdirs() as mkdirs() does that anyway.

There is similar code at [2] and possibly elsewhere in Tomcat, I did not check.

[1] http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/juli/FileHandler.java?view=markup#l364

[1] http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/juli/FileHandler.java?view=markup#l379
Comment 1 Mark Thomas 2011-08-26 17:12:51 UTC
There were ~25 uses of mkdirs() throughout the codebase. All have been reviewed and fixed as appropriate.

If we are being this pedantic then

if (!dir.mkdirs() && !dir.isDirectory()) {

is a better test since it ensures the file that exists is a directory rather than a regular file.

This has been fixed in trunk and 7.0.x and will be included in 7.0.21 onwards.