Commons JCI
  1. Commons JCI
  2. JCI-67

Dubious use of mkdirs() return code

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.1
    • Component/s: None
    • Labels:
      None

      Description

      FileRestoreStore.java uses mkdirs() as follows:

      final File parent = file.getParentFile();
      if (!parent.exists()) {
          if (!parent.mkdirs()) {
              throw new IOException("could not create" + parent);
          }
      }
      

      Now mkdirs() returns true only if the method actually created the directories; it's theoretically possible for the directory to be created in the window between the exists() and mkdirs() invocations.

      Also, the initial exists() call is redundant, because that's what mkdirs() does anyway (in the RI implementation, at least).

      I suggest the following instead:

      final File parent = file.getParentFile();
      if (!parent.mkdirs() && !parent.exists()) {
              throw new IOException("could not create" + parent);
          }
      }
      

      If mkdirs() returns false, the code then checks to see if the directory exists, so the throws clause will only be invoked if the parent really cannot be created.

      The same code also appears in AbstractTestCase and FilesystemAlterationMonitorTestCase.

        Issue Links

          Activity

          Sebb created issue -
          Hide
          Sebb added a comment - - edited

          Safer would be the following, as it checks the path is actually a directory:

          final File parent = file.getParentFile();
          if (!parent.mkdirs() && !parent.isDirectory()) {
              throw new IOException("could not create" + parent);
          }
          
          Show
          Sebb added a comment - - edited Safer would be the following, as it checks the path is actually a directory: final File parent = file.getParentFile(); if (!parent.mkdirs() && !parent.isDirectory()) { throw new IOException( "could not create" + parent); }
          Sebb made changes -
          Field Original Value New Value
          Link This issue is related to IO-280 [ IO-280 ]
          Emmanuel Bourg made changes -
          Fix Version/s 1.1 [ 12312668 ]
          Hide
          Emmanuel Bourg added a comment -

          Sounds good to me.

          Show
          Emmanuel Bourg added a comment - Sounds good to me.
          Hide
          Sebb added a comment -

          URL: http://svn.apache.org/r1517840
          Log:
          JCI-67 Dubious use of mkdirs() return code

          Modified:
          commons/proper/jci/trunk/core/src/main/java/org/apache/commons/jci/stores/FileResourceStore.java
          commons/proper/jci/trunk/core/src/test/java/org/apache/commons/jci/AbstractTestCase.java
          commons/proper/jci/trunk/fam/src/test/java/org/apache/commons/jci/monitor/FilesystemAlterationMonitorTestCase.java

          Show
          Sebb added a comment - URL: http://svn.apache.org/r1517840 Log: JCI-67 Dubious use of mkdirs() return code Modified: commons/proper/jci/trunk/core/src/main/java/org/apache/commons/jci/stores/FileResourceStore.java commons/proper/jci/trunk/core/src/test/java/org/apache/commons/jci/AbstractTestCase.java commons/proper/jci/trunk/fam/src/test/java/org/apache/commons/jci/monitor/FilesystemAlterationMonitorTestCase.java
          Sebb made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Emmanuel Bourg made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Unassigned
              Reporter:
              Sebb
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development