Commons Compress
  1. Commons Compress
  2. COMPRESS-26

TarArchiveEntry(File) now crashes on file system roots

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.0
    • Component/s: None
    • Labels:
      None
    • Environment:

      win xp sp3, but this is probably irrelevant

      Description

      The TarArchiveEntry(File) constructor now crashes if the File argument is a file system root.

      For example, on my windows box, I want to backup the entire contents of my F drive, so I am supplying a File argument that is constructed as
      new File("F:
      ")

      That particular file causes the TarArchiveEntry(File) constructor to fail as follows:
      Caused by: java.lang.StringIndexOutOfBoundsException: String index out of range: -1
      at java.lang.StringBuffer.charAt(StringBuffer.java:162)
      at org.apache.commons.compress.archivers.tar.TarArchiveEntry.<init>(TarArchiveEntry.java:245)

      Looking at the code (I downloaded revision 743098 yesterday), it is easy to see why this occured:

      1) the
      if (osname != null) {
      logic will strip the "F:" from my path name of "F:\", leaving just the "\"

      2) that "\" will then be turned into a single "/" by the
      fileName = fileName.replace(File.separatorChar, '/');
      line

      3) that single "/" will then be removed by the
      while (fileName.startsWith("/")) {
      logic, leaving the empty string "".

      4) then line #245
      if (this.name.charAt(this.name.length() - 1) != '/') {
      must crash, because it falsely assumes that fileName has content.

      THIS IS A SHOW STOPPER BUG FOR ME.

      I am not sure when this current behavior of TarArchiveEntry was introduced; a very old codebase (from 2+ years ago) of compress that I used to use handled file system roots just fine.

      There are many ways to fix this. For instance, if it is, in fact, OK for the name field to be empty, then you can simply put a check on line #245 as follows:
      if ( (name.length() > 0) && (name.charAt(name.length() - 1) != '/') ) {
      (NOTE on coding style: do you really need to use "this." in the constructor when there is no possible ambiguity? Makes your code wordier and therefore harder to read.)

      My guess, not knowing your full codebase well, is that it is NOT OK for name to be blank. For example, you seem to want directories to end with a '/' char, and file ssystem roots are always directories.

      Therefore, you have some decisions to make:

      a) is it OK for the name field to simply be "/" in the case of file system roots?

      b) if a) is not good for some reason, then you must introduce an artificial root name, so that name takes on a value like
      "filesystemRoot/"
      or
      "filesystemRoot_F/"
      or whatever.

      This bug, by the way, brings up another issue: there currently are no javadocs regarding field contracts. Every field's javadocs needs its constraints to be specified as a contract, for example,

      /**

      • The entry's name.
      • <p>
      • Contract: is never null (and never empty?).
      • Contains (only ASCII chars? any Unicode chars?).
      • Must be (<= 100 chars? unlimited number of chars?).
      • If {@link #file}

        is a directory, then must end in a '/' char.

      • etc...
        */
        private StringBuffer name;
      1. patch-filerootcrash.txt
        16 kB
        Christian Grobmeier

        Activity

        Hide
        Stefan Bodewig added a comment -

        svn revision 755227

        Show
        Stefan Bodewig added a comment - svn revision 755227
        Hide
        Stefan Bodewig added a comment -

        The code has been that way since the very first version in Ant

        http://svn.eu.apache.org/viewvc/ant/core/trunk/src/main/org/apache/tools/tar/TarEntry.java?revision=267597&view=markup

        Strangely, it only affects the File-arg constructor, maybe you've been using the String-arg constructor in the past.

        I'm currently writing a few tests to see whether a file name of "/" causes any problems (GNU tar complains but simply strips the leading /) and will check in a fix - that applies to all constructors - soon.

        Show
        Stefan Bodewig added a comment - The code has been that way since the very first version in Ant http://svn.eu.apache.org/viewvc/ant/core/trunk/src/main/org/apache/tools/tar/TarEntry.java?revision=267597&view=markup Strangely, it only affects the File-arg constructor, maybe you've been using the String-arg constructor in the past. I'm currently writing a few tests to see whether a file name of "/" causes any problems (GNU tar complains but simply strips the leading /) and will check in a fix - that applies to all constructors - soon.
        Hide
        Christian Grobmeier added a comment -

        Attached patch should fix the problem. Before:
        this.name.charAt(this.name.length() - 1) != '/'

        name's length will be asked if it is zero. If so, the root directory is meant.

        I couldn't write a working test since I am on OSX. I had to copy out the necessary code. If somebody has an Win-Box, please test.

        I think this file needs some refactoring to make it more testable.

        Show
        Christian Grobmeier added a comment - Attached patch should fix the problem. Before: this.name.charAt(this.name.length() - 1) != '/' name's length will be asked if it is zero. If so, the root directory is meant. I couldn't write a working test since I am on OSX. I had to copy out the necessary code. If somebody has an Win-Box, please test. I think this file needs some refactoring to make it more testable.

          People

          • Assignee:
            Stefan Bodewig
            Reporter:
            Sam Smith
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development