Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-181

INode.getPathComponents throws NPE when given a non-absolute path

    Details

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

      Description

      If you pass a path that doesn't start with '/' to INode.getPathComponents, it throws a NullPointerException. Instead it should throw IllegalArgumentException to make it clear that absolute paths are required in this code.

      The attached patch fixes this, clarifies, the javadoc, and adds a test case.

      1. HADOOP-5700.txt
        3 kB
        Todd Lipcon
      2. HADOOP-5700-v2.txt
        2 kB
        Todd Lipcon
      3. HADOOP-5700-v2b.txt
        2 kB
        Tsz Wo Nicholas Sze

        Issue Links

          Activity

          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12405794/HADOOP-5700.txt
          against trunk revision 767331.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/224/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/224/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/224/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/224/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12405794/HADOOP-5700.txt against trunk revision 767331. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/224/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/224/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/224/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/224/console This message is automatically generated.
          Hide
          Tom White added a comment -

          I've just committed this. Thanks Todd!

          Show
          Tom White added a comment - I've just committed this. Thanks Todd!
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Questions:

          • How to reproduce the problem? It seems to me that getPathComponents(..) will return null but not throwing NPE before the patch.
          • Did anyone measure the performance impact on the patch? The check (!path.startsWith("/")) introduced by the patch seems to be redundant since getPathNames(..) has the same check.
          • After the patch, calling getPathComponents(null) will get a NPE but it was not before the patch. Is it a bug?
          Show
          Tsz Wo Nicholas Sze added a comment - Questions: How to reproduce the problem? It seems to me that getPathComponents(..) will return null but not throwing NPE before the patch. Did anyone measure the performance impact on the patch? The check (!path.startsWith("/")) introduced by the patch seems to be redundant since getPathNames(..) has the same check. After the patch, calling getPathComponents(null) will get a NPE but it was not before the patch. Is it a bug?
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > How to reproduce the problem? It seems to me that getPathComponents(..) will return null but not throwing NPE before the patch.
          I was wrong about this: getPathNames(..) will return null but not getPathComponents(..), which will throw NPE.

          Show
          Tsz Wo Nicholas Sze added a comment - > How to reproduce the problem? It seems to me that getPathComponents(..) will return null but not throwing NPE before the patch. I was wrong about this: getPathNames(..) will return null but not getPathComponents(..), which will throw NPE.
          Hide
          Todd Lipcon added a comment -

          Did anyone measure the performance impact on the patch? The check (!path.startsWith("/")) introduced by the patch seems to be redundant since getPathNames(..) has the same check.

          I can't imagine this would have any discernible performance impact, since it simply accesses the first character of an existing String. This is nothing compared to the allocation and byte copying that occurs inside of getPathComponents.

          After the patch, calling getPathComponents(null) will get a NPE but it was not before the patch. Is it a bug?

          This isn't changed by the patch - previously getPathNames returned null, which was then passed into getPathComponents. This would have thrown an NPE when accessing strings.length. In either case, if you pass null into a method which doesn't explicitly document that as being acceptable, you should not be surprised by an NPE.

          Show
          Todd Lipcon added a comment - Did anyone measure the performance impact on the patch? The check (!path.startsWith("/")) introduced by the patch seems to be redundant since getPathNames(..) has the same check. I can't imagine this would have any discernible performance impact, since it simply accesses the first character of an existing String. This is nothing compared to the allocation and byte copying that occurs inside of getPathComponents. After the patch, calling getPathComponents(null) will get a NPE but it was not before the patch. Is it a bug? This isn't changed by the patch - previously getPathNames returned null, which was then passed into getPathComponents. This would have thrown an NPE when accessing strings.length. In either case, if you pass null into a method which doesn't explicitly document that as being acceptable, you should not be surprised by an NPE.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          You are right that the patch does not introduce new bugs. Thanks, Todd. I was concerned about the redundant checks. For internally methods like getPathComponents(..), the parameters are often not validated for performance reason. The caller should make sure the actual parameters are correct.

          Show
          Tsz Wo Nicholas Sze added a comment - You are right that the patch does not introduce new bugs. Thanks, Todd. I was concerned about the redundant checks. For internally methods like getPathComponents(..), the parameters are often not validated for performance reason. The caller should make sure the actual parameters are correct.
          Hide
          Raghu Angadi added a comment -

          +1 for javadoc fix.

          Regd throwing a runtime exception, is it really different from NPE? Wouldn't any runtime exception in NN indicate a bug in NN?

          Can a user commad/usercode actually trigger this NPE? If yes, then it probably should be regular error (IOException etc) rather than a runtime exception.

          How did you notice this NPE?

          Show
          Raghu Angadi added a comment - +1 for javadoc fix. Regd throwing a runtime exception, is it really different from NPE? Wouldn't any runtime exception in NN indicate a bug in NN? Can a user commad/usercode actually trigger this NPE? If yes, then it probably should be regular error (IOException etc) rather than a runtime exception. How did you notice this NPE?
          Hide
          Todd Lipcon added a comment -

          Tsz Wo said:

          For internally methods like getPathComponents(..), the parameters are often not validated for performance reason

          While I appreciate the importance of performance, I think in all but the very tightest loops, constant-time (O(1)) parameter validation is worth it. NN metadata ops are almost never the bottleneck of a cluster, and I can't imagine a case where an extra memory access or two would comprise a measurable portion of request latency. Given this, I think it's far more important to focus on stability, reasonable error messages, and preventing regressions due to API misuse than it is to worry about extra O(1) method calls.

          I do agree that parameter validation should be happening at the outer layers of the API, but since I somehow managed to get the NPE at this point, I figured this API was somehow publicly accessible. See below.

          Raghu said:

          Regd throwing a runtime exception, is it really different from NPE? Wouldn't any runtime exception in NN indicate a bug in NN?

          Yes. For me, the difference in IAE vs NPE is that it's explicitly "intended" and offers some helpful explanation to the developer about what went wrong in their code. An NPE on the other hand could be an unknown internal bug, or a misuse of API, or any number of other things.

          Can a user commad/usercode actually trigger this NPE? If yes, then it probably should be regular error (IOException etc) rather than a runtime exception.

          You can trigger this NPE if you have a reference to the NameNode (through ClientProtocol). It looks to me like you could even trigger it by DFSClient.exists or DFSClient.getFileInfo, though I'd have to write a test case. If people think this error should be caught in either NameNode, FSNamesystem, or FSDirectory, then I agree - just let me know which place makes the most sense and I'll move the code there and write an appropriate unit test.

          How did you notice this NPE?

          I triggered it by calling namenode.getFileInfo() on a relative path from within my thrift contrib code (HADOOP-4707). This was an error in my code, since I was misusing the API, but it took me some serious digging to discover that the lack of initial '/' is what caused it. If the error had been the IAE instead I would have found my error much quicker.

          Show
          Todd Lipcon added a comment - Tsz Wo said: For internally methods like getPathComponents(..), the parameters are often not validated for performance reason While I appreciate the importance of performance, I think in all but the very tightest loops, constant-time (O(1)) parameter validation is worth it. NN metadata ops are almost never the bottleneck of a cluster, and I can't imagine a case where an extra memory access or two would comprise a measurable portion of request latency. Given this, I think it's far more important to focus on stability, reasonable error messages, and preventing regressions due to API misuse than it is to worry about extra O(1) method calls. I do agree that parameter validation should be happening at the outer layers of the API, but since I somehow managed to get the NPE at this point, I figured this API was somehow publicly accessible. See below. Raghu said: Regd throwing a runtime exception, is it really different from NPE? Wouldn't any runtime exception in NN indicate a bug in NN? Yes. For me, the difference in IAE vs NPE is that it's explicitly "intended" and offers some helpful explanation to the developer about what went wrong in their code. An NPE on the other hand could be an unknown internal bug, or a misuse of API, or any number of other things. Can a user commad/usercode actually trigger this NPE? If yes, then it probably should be regular error (IOException etc) rather than a runtime exception. You can trigger this NPE if you have a reference to the NameNode (through ClientProtocol). It looks to me like you could even trigger it by DFSClient.exists or DFSClient.getFileInfo, though I'd have to write a test case. If people think this error should be caught in either NameNode, FSNamesystem, or FSDirectory, then I agree - just let me know which place makes the most sense and I'll move the code there and write an appropriate unit test. How did you notice this NPE? I triggered it by calling namenode.getFileInfo() on a relative path from within my thrift contrib code ( HADOOP-4707 ). This was an error in my code, since I was misusing the API, but it took me some serious digging to discover that the lack of initial '/' is what caused it. If the error had been the IAE instead I would have found my error much quicker.
          Hide
          Raghu Angadi added a comment -

          > I triggered it by calling namenode.getFileInfo() on a relative path from within my thrift contrib code (HADOOP-4707).

          Thanks Todd. That implies user can trigger it. So namenode.getFileInfo(null) might still trigger an NPE even after the patch.

          Do you think NN should sanity check user input at the highest level (in NameNode.java) and throw an IOException?
          Or in this particular case, not having "/" at the beginning is like giving random string for path.. which should trigger "FileNotFoundException" and not a runtime exception IMO. But another IOException with helpful message is fine too.

          Show
          Raghu Angadi added a comment - > I triggered it by calling namenode.getFileInfo() on a relative path from within my thrift contrib code ( HADOOP-4707 ). Thanks Todd. That implies user can trigger it. So namenode.getFileInfo(null) might still trigger an NPE even after the patch. Do you think NN should sanity check user input at the highest level (in NameNode.java) and throw an IOException? Or in this particular case, not having "/" at the beginning is like giving random string for path.. which should trigger "FileNotFoundException" and not a runtime exception IMO. But another IOException with helpful message is fine too.
          Hide
          Raghu Angadi added a comment -

          In your case, if getPathNames() returned 'new String[0];' rather than null, would the it work fine? (i.e. namenode.getFileInfo() behaves as though a non-existent path was queried).

          But that still leaves the problem with namenode.getFileInfo(null).

          Show
          Raghu Angadi added a comment - In your case, if getPathNames() returned 'new String [0] ;' rather than null, would the it work fine? (i.e. namenode.getFileInfo() behaves as though a non-existent path was queried). But that still leaves the problem with namenode.getFileInfo(null).
          Hide
          Todd Lipcon added a comment -

          In your case, if getPathNames() returned 'new String[0];' rather than null, would the it work fine? (i.e. namenode.getFileInfo() behaves as though a non-existent path was queried).

          Well, I would actually prefer either an IllegalArgumentException or an IOException that explicitly says ("Argument to getFileInfo should be an absolute path") rather than a vanilla FileNotFound exception. This makes it clear to the user-developer that they are misusing the API rather than just looking for a nonexistent file.

          But that still leaves the problem with namenode.getFileInfo(null).

          I think if people pass a null into a function they should expect it to break. That's an easy thing to diagnose from the user-developer perspective, whereas missing initial '/' is a bit trickier.

          I'd love to include something like the google Preconditions class for cases like this. It cuts down on code cruft and makes it clear what is going on:

          http://google-collections.googlecode.com/svn/trunk/javadoc/com/google/common/base/Preconditions.html

          Show
          Todd Lipcon added a comment - In your case, if getPathNames() returned 'new String [0] ;' rather than null, would the it work fine? (i.e. namenode.getFileInfo() behaves as though a non-existent path was queried). Well, I would actually prefer either an IllegalArgumentException or an IOException that explicitly says ("Argument to getFileInfo should be an absolute path") rather than a vanilla FileNotFound exception. This makes it clear to the user-developer that they are misusing the API rather than just looking for a nonexistent file. But that still leaves the problem with namenode.getFileInfo(null). I think if people pass a null into a function they should expect it to break. That's an easy thing to diagnose from the user-developer perspective, whereas missing initial '/' is a bit trickier. I'd love to include something like the google Preconditions class for cases like this. It cuts down on code cruft and makes it clear what is going on: http://google-collections.googlecode.com/svn/trunk/javadoc/com/google/common/base/Preconditions.html
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > ... and I can't imagine a case where an extra memory access or two would comprise a measurable portion of request latency.

          You are right that this single patch is not going to affect the performance much but you may imagine that if all the internal methods validate the parameters, it probably will hurt.

          The question is: why validation is required in this particular method, getPathComponents(..), but not the others?

          > I do agree that parameter validation should be happening at the outer layers of the API, but since I somehow managed to get the NPE at this point, I figured this API was somehow publicly accessible...

          In this case, the outer API seems to be getFileInfo(..) but not getPathComponents(..). Why not fixing getFileInfo(..)?

          Show
          Tsz Wo Nicholas Sze added a comment - > ... and I can't imagine a case where an extra memory access or two would comprise a measurable portion of request latency. You are right that this single patch is not going to affect the performance much but you may imagine that if all the internal methods validate the parameters, it probably will hurt. The question is: why validation is required in this particular method, getPathComponents(..), but not the others? > I do agree that parameter validation should be happening at the outer layers of the API, but since I somehow managed to get the NPE at this point, I figured this API was somehow publicly accessible... In this case, the outer API seems to be getFileInfo(..) but not getPathComponents(..). Why not fixing getFileInfo(..)?
          Hide
          Tsz Wo Nicholas Sze added a comment -

          After some serious thought, I suggest to revert the patch committed for the following reasons:

          • The patch introduces a redundant check, which degrades the namenode performance. All production systems will suffer this. The performance impact is still unknown since no performance measurement is even done.
          • As mentioned by Todd's earlier comment, parameter validation should be happening at the outer layers of the API but not internal methods. However, the patch does parameter validation in INode.getPathComponents(..) which is an internal method.
          • The patch uses "/" but not the Path.SEPARATOR constant. It is incorrect, strictly speaking.
          • The patch does not fix any bug: it changes NPE to IAE. Both of them are uncaught runtime exceptions.

          Todd, Tom, Raghu, what do you think?

          Show
          Tsz Wo Nicholas Sze added a comment - After some serious thought, I suggest to revert the patch committed for the following reasons: The patch introduces a redundant check, which degrades the namenode performance. All production systems will suffer this. The performance impact is still unknown since no performance measurement is even done. As mentioned by Todd's earlier comment , parameter validation should be happening at the outer layers of the API but not internal methods. However, the patch does parameter validation in INode.getPathComponents(..) which is an internal method. The patch uses "/" but not the Path.SEPARATOR constant. It is incorrect, strictly speaking. The patch does not fix any bug: it changes NPE to IAE. Both of them are uncaught runtime exceptions. Todd, Tom, Raghu, what do you think?
          Hide
          Todd Lipcon added a comment -

          The patch introduces a redundant check, which degrades the namenode performance. All production systems will suffer this. The performance impact is still unknown since no performance measurement is even done.

          I strongly disagree on the performance issue. I'd go into detail about why this cannot possibly cause a performance problem, but clearly this is an idealogical debate rather than a practical one for this case. In short: the data accessed is about to be split, meaning that it can't be an extra cache miss, and the splitting and allocation will be several of orders of magnitude slower than a constant-time single-byte comparison.

          The patch uses "/" but not the Path.SEPARATOR constant. It is incorrect, strictly speaking.

          Good point. That should be fixed.

          The patch does not fix any bug: it changes NPE to IAE. Both of them are uncaught runtime exceptions

          I think of IAEs as stricly more informative than NPEs for the developer who has committed the error. Again, no sense continuing that discussion.

          As mentioned by Todd's earlier comment, parameter validation should be happening at the outer layers of the API but not internal methods. However, the patch does parameter validation in INode.getPathComponents(..) which is an internal method.

          Also should be fixed.

          I'll prepare a patch to address the above things.

          On a side note, what is the policy for dealing with updated patches after a JIRA has already been committed? As far as I knew, discussion of a better/new patch is supposed to go on in a new JIRA.

          Show
          Todd Lipcon added a comment - The patch introduces a redundant check, which degrades the namenode performance. All production systems will suffer this. The performance impact is still unknown since no performance measurement is even done. I strongly disagree on the performance issue. I'd go into detail about why this cannot possibly cause a performance problem, but clearly this is an idealogical debate rather than a practical one for this case. In short: the data accessed is about to be split, meaning that it can't be an extra cache miss, and the splitting and allocation will be several of orders of magnitude slower than a constant-time single-byte comparison. The patch uses "/" but not the Path.SEPARATOR constant. It is incorrect, strictly speaking. Good point. That should be fixed. The patch does not fix any bug: it changes NPE to IAE. Both of them are uncaught runtime exceptions I think of IAEs as stricly more informative than NPEs for the developer who has committed the error. Again, no sense continuing that discussion. As mentioned by Todd's earlier comment, parameter validation should be happening at the outer layers of the API but not internal methods. However, the patch does parameter validation in INode.getPathComponents(..) which is an internal method. Also should be fixed. I'll prepare a patch to address the above things. On a side note, what is the policy for dealing with updated patches after a JIRA has already been committed? As far as I knew, discussion of a better/new patch is supposed to go on in a new JIRA.
          Hide
          Raghu Angadi added a comment -

          I didn't notice till now that this was committed. My opinion :

          -1 for runtime exception
          +0.5 for IOException (with helpful message)
          +1 same as non-existent path (ie. sometimes an exception and sometimes not, upper levels decide that)

          I understand how IAE helped in this particular case. we have all seen silly errors that take long time to debug sometimes. IMO what exception to be thrown should not be based on that (but the message could be made more helpful).

          Show
          Raghu Angadi added a comment - I didn't notice till now that this was committed. My opinion : -1 for runtime exception +0.5 for IOException (with helpful message) +1 same as non-existent path (ie. sometimes an exception and sometimes not, upper levels decide that) I understand how IAE helped in this particular case. we have all seen silly errors that take long time to debug sometimes. IMO what exception to be thrown should not be based on that (but the message could be made more helpful).
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I'll prepare a patch to address the above things.

          On a side note, what is the policy for dealing with updated patches after a JIRA has already been committed? As far as I knew, discussion of a better/new patch is supposed to go on in a new JIRA.

          Thanks, Todd. I think the right way is to revert the committed patch and work on a new patch in this issue.

          I strongly disagree on the performance issue. I'd go into detail about why this cannot possibly cause a performance problem, but clearly this is an idealogical debate rather than a practical one for this case.

          For practical reasons, we need numbers. Without measurement, this becomes a debate. getPathComponents(..) is one of the central internal methods in Namenode. All accesses to the namespace use it. Many of the accesses acquire the namespace lock beforehand. Please try measuring the time required for lsr on a directory with 10 millions descendants before and after the patch.

          ... a constant-time single-byte comparison.

          When we check the code of String.startWith(String), it is not a single-byte comparison. If the patch uses the return-value of getPathNames(..), I won't have problems on it.

          I think of IAEs as stricly more informative than NPEs for the developer who has committed the error.

          I agree that IAEs is more informative than NPEs for the developers but it is never worth to benifit developers by sacrificing the performance of production systems.

          Show
          Tsz Wo Nicholas Sze added a comment - I'll prepare a patch to address the above things. On a side note, what is the policy for dealing with updated patches after a JIRA has already been committed? As far as I knew, discussion of a better/new patch is supposed to go on in a new JIRA. Thanks, Todd. I think the right way is to revert the committed patch and work on a new patch in this issue. I strongly disagree on the performance issue. I'd go into detail about why this cannot possibly cause a performance problem, but clearly this is an idealogical debate rather than a practical one for this case. For practical reasons, we need numbers. Without measurement, this becomes a debate. getPathComponents(..) is one of the central internal methods in Namenode. All accesses to the namespace use it. Many of the accesses acquire the namespace lock beforehand. Please try measuring the time required for lsr on a directory with 10 millions descendants before and after the patch. ... a constant-time single-byte comparison. When we check the code of String.startWith(String), it is not a single-byte comparison. If the patch uses the return-value of getPathNames(..), I won't have problems on it. I think of IAEs as stricly more informative than NPEs for the developer who has committed the error. I agree that IAEs is more informative than NPEs for the developers but it is never worth to benifit developers by sacrificing the performance of production systems.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I am reverting it.

          Show
          Tsz Wo Nicholas Sze added a comment - I am reverting it.
          Hide
          Konstantin Shvachko added a comment -

          I agree with Nicholas. We should reverse the patch.
          The problem is solved by a simple one line change:

          /** Convert strings to byte arrays for path components. */
          static byte[][] getPathComponents(String[] strings) {
          -  if (strings.length == 0) {
          +  if (strings == null || strings.length == 0) {
              return new byte[][]{null};
            }
          
          Show
          Konstantin Shvachko added a comment - I agree with Nicholas. We should reverse the patch. The problem is solved by a simple one line change: /** Convert strings to byte arrays for path components. */ static byte [][] getPathComponents( String [] strings) { - if (strings.length == 0) { + if (strings == null || strings.length == 0) { return new byte [][]{ null }; }
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Here is my suggested fix:

             static byte[][] getPathComponents(String path) {
          -    return getPathComponents(getPathNames(path));
          +    final String[] strings = getPathNames(path);
          +    if (strings == null) {
          +      throw new IllegalArgumentException(path == null? "path == null"
          +          : "Must pass an absolute path to getPathComponents, path=" + path);
          +    }
          +    return getPathComponents(strings);
             }
          
          Show
          Tsz Wo Nicholas Sze added a comment - Here is my suggested fix: static byte [][] getPathComponents( String path) { - return getPathComponents(getPathNames(path)); + final String [] strings = getPathNames(path); + if (strings == null ) { + throw new IllegalArgumentException(path == null ? "path == null " + : "Must pass an absolute path to getPathComponents, path=" + path); + } + return getPathComponents(strings); }
          Hide
          Konstantin Shvachko added a comment -

          No, RuntimeException is an indication of a bug. And replacing one bug by another does not solve the problem.
          I think in addition to what I proposed we should also call DFSUtil.isValidName(src) for all methods in FSNamesystem which take a path name string as a parameter. This is done correctly in startFileInternal(), renameToInternal() and mkdirsInternal(), but not in getFileInfo() or getBlockLocations().
          I propose to fix this jira as I proposed, if it works of course, which I didn't check. And create a new one, which would make sure DFSUtil.isValidName(src) is called everywhere it should be.

          Show
          Konstantin Shvachko added a comment - No, RuntimeException is an indication of a bug. And replacing one bug by another does not solve the problem. I think in addition to what I proposed we should also call DFSUtil.isValidName(src) for all methods in FSNamesystem which take a path name string as a parameter. This is done correctly in startFileInternal() , renameToInternal() and mkdirsInternal() , but not in getFileInfo() or getBlockLocations() . I propose to fix this jira as I proposed, if it works of course, which I didn't check. And create a new one, which would make sure DFSUtil.isValidName(src) is called everywhere it should be.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > No, RuntimeException is an indication of a bug. And replacing one bug by another does not solve the problem.

          As mentioned before, I don't think there is a bug in getPathComponents(..) since the parameter is supposed to be an absolute path. Throwing RuntimeException here indicates that the caller codes have a bug.

          > ... I proposed we should also call DFSUtil.isValidName(src) ...

          Yes, this is another option of fixing this.

          Show
          Tsz Wo Nicholas Sze added a comment - > No, RuntimeException is an indication of a bug. And replacing one bug by another does not solve the problem. As mentioned before, I don't think there is a bug in getPathComponents(..) since the parameter is supposed to be an absolute path. Throwing RuntimeException here indicates that the caller codes have a bug. > ... I proposed we should also call DFSUtil.isValidName(src) ... Yes, this is another option of fixing this.
          Hide
          Todd Lipcon added a comment -

          Here's a new crack at this patch that adds the validation at the outer layer of the API. Also includes an improvement to the unit test to verify this behavior.

          Show
          Todd Lipcon added a comment - Here's a new crack at this patch that adds the validation at the outer layer of the API. Also includes an improvement to the unit test to verify this behavior.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 the patch is good. Thanks, Todd!

          Show
          Tsz Wo Nicholas Sze added a comment - +1 the patch is good. Thanks, Todd!
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12410558/HADOOP-5700-v2.txt
          against trunk revision 785065.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          -1 contrib tests. The patch failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/510/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/510/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/510/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/510/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12410558/HADOOP-5700-v2.txt against trunk revision 785065. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/510/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/510/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/510/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/510/console This message is automatically generated.
          Hide
          steve_l added a comment -

          I've just marked HADOOP-6028 as a duplicate. What is interesting there is that you can trigger the NPE by browsing to http://namenode/fileChecksum/* ; the end user gets to see some error XML while the stack trace gets created server side.

          This adds a new functional test, incidentally, try to GET that URL on a live system.

          Show
          steve_l added a comment - I've just marked HADOOP-6028 as a duplicate. What is interesting there is that you can trigger the NPE by browsing to http://namenode/fileChecksum/* ; the end user gets to see some error XML while the stack trace gets created server side. This adds a new functional test, incidentally, try to GET that URL on a live system.
          Hide
          Todd Lipcon added a comment -

          The failing contrib test is unrelated. Think this is good to go once the "no commits on trunk until project split" mutex is released

          Show
          Todd Lipcon added a comment - The failing contrib test is unrelated. Think this is good to go once the "no commits on trunk until project split" mutex is released
          Hide
          Tsz Wo Nicholas Sze added a comment -

          HADOOP-5700-v2b.txt: same patch for the new project structure.

          Show
          Tsz Wo Nicholas Sze added a comment - HADOOP-5700 -v2b.txt: same patch for the new project structure.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have committed this. Thanks, Todd!

          Show
          Tsz Wo Nicholas Sze added a comment - I have committed this. Thanks, Todd!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #8 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/8/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #8 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/8/ )

            People

            • Assignee:
              Todd Lipcon
              Reporter:
              Todd Lipcon
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development