Uploaded image for project: 'Chukwa'
  1. Chukwa
  2. CHUKWA-642

Unvalidated Regular Expression Usage

    Details

      Description

      There are seven additional places in Chukwa where regular expressions are used without first being validated as syntactically valid regular expressions. These could lead to unhelpful PatternSyntaxException strack traces instead of useful error messages. Unfortunately, I was not able to produce test conditions to highlight any of these issues.

      The attached patch fixes the issues. Note that the patch adds a small RegexUtil class with helper methods to determine whether a String is a valid regular expression and to generate error messages for invalid regular expressions. I feel that these helper methods are more readable than catching the PatternSyntaxException. Furthermore, they allow simpler re-use if needed elsewhere.

      I discovered these bugs using a tool named the Regex Checker (http://types.cs.washington.edu/checker-framework/current/checkers-manual.html#regex-checker). If you periodically run it on your codebase, then you will learn of other errors like this as soon as they appear. If you are interested, I can share my annotations for Chukwa, which will enable you to run the Regex Checker yourself without any additional effort.

      1. CHUKWA-642.patch
        12 kB
        Eric Spishak
      2. CHUKWA-642-1.patch
        12 kB
        Eric Spishak
      3. CHUKWA-642-2.patch
        28 kB
        Eric Spishak

        Activity

        Hide
        eyang Eric Yang added a comment -

        It might be too harsh to call System.exit on regular expression errors. It would be nice if the logging is changed to warning, and skip to next operation.

        Show
        eyang Eric Yang added a comment - It might be too harsh to call System.exit on regular expression errors. It would be nice if the logging is changed to warning, and skip to next operation.
        Hide
        espishak Eric Spishak added a comment -

        Sorry about the delay in getting back to you. I've updated the patch to handle errors more nicely. Please let me know if I should make any more changes.

        Show
        espishak Eric Spishak added a comment - Sorry about the delay in getting back to you. I've updated the patch to handle errors more nicely. Please let me know if I should make any more changes.
        Hide
        eyang Eric Yang added a comment -

        Could you add a test case? Thanks

        Show
        eyang Eric Yang added a comment - Could you add a test case? Thanks
        Hide
        espishak Eric Spishak added a comment -

        Hi Eric, thanks for getting back to me. There's actually 7 bugs here. Would you like me to add a unit test for each of these? Or an end-to-end test case (for example, a command on the command line that generates a crash) for each?

        Show
        espishak Eric Spishak added a comment - Hi Eric, thanks for getting back to me. There's actually 7 bugs here. Would you like me to add a unit test for each of these? Or an end-to-end test case (for example, a command on the command line that generates a crash) for each?
        Hide
        eyang Eric Yang added a comment -

        Unit test case for each of the 7 bugs would help. Thanks

        Show
        eyang Eric Yang added a comment - Unit test case for each of the 7 bugs would help. Thanks
        Hide
        espishak Eric Spishak added a comment -

        I just attached a new patch with unit tests added.

        I was not able to add tests for the changes in org/apache/hadoop/chukwa/database/TableCreator.java, org/apache/hadoop/chukwa/database/DataExpiration.java and org/apache/hadoop/chukwa/dataloader/MetricDataLoader.java because these rely on the existence of a jdbc.conf file, which does not come with Chukwa. I also noticed that the tests for these classes were excluded from the Maven test task.

        And I was not able to add tests for org/apache/hadoop/chukwa/datacollection/writer/SocketTeeWriter.java because the modified code runs in a different thread than the test, so I had trouble figuring out how to verify the correct behavior.

        If you have suggestions for how to get around these problems I will gladly add additional test cases.

        Show
        espishak Eric Spishak added a comment - I just attached a new patch with unit tests added. I was not able to add tests for the changes in org/apache/hadoop/chukwa/database/TableCreator.java, org/apache/hadoop/chukwa/database/DataExpiration.java and org/apache/hadoop/chukwa/dataloader/MetricDataLoader.java because these rely on the existence of a jdbc.conf file, which does not come with Chukwa. I also noticed that the tests for these classes were excluded from the Maven test task. And I was not able to add tests for org/apache/hadoop/chukwa/datacollection/writer/SocketTeeWriter.java because the modified code runs in a different thread than the test, so I had trouble figuring out how to verify the correct behavior. If you have suggestions for how to get around these problems I will gladly add additional test cases.
        Hide
        eyang Eric Yang added a comment -

        TableCreator.java, DataExpiration.java and MetricDataLoader.java are legacy artifact, and should be removed in Chukwa 0.6.0. It is fine that this patch does not modify those files. The patch should be fine as it is. I will commit this week, if there is no objection.

        Show
        eyang Eric Yang added a comment - TableCreator.java, DataExpiration.java and MetricDataLoader.java are legacy artifact, and should be removed in Chukwa 0.6.0. It is fine that this patch does not modify those files. The patch should be fine as it is. I will commit this week, if there is no objection.
        Hide
        eyang Eric Yang added a comment -

        I just committed this, thanks Eric.

        Show
        eyang Eric Yang added a comment - I just committed this, thanks Eric.
        Hide
        hudson Hudson added a comment -

        Integrated in Chukwa-trunk #460 (See https://builds.apache.org/job/Chukwa-trunk/460/)
        CHUKWA-642. Added regular expression validation. (Eric Spishak via Eric Yang) (Revision 1411833)
        CHUKWA-642. Added regular expression validation. (Eric Spishak via Eric Yang) (Revision 1411813)

        Result = SUCCESS
        eyang : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1411833
        Files :

        • /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/util/RegexUtil.java

        eyang : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1411813
        Files :

        • /incubator/chukwa/trunk/CHANGES.txt
        • /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/database/DataExpiration.java
        • /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/database/TableCreator.java
        • /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/datacollection/writer/SocketTeeWriter.java
        • /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/dataloader/MetricDataLoader.java
        • /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/extraction/demux/processor/mapper/TsProcessor.java
        • /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/inputtools/ChukwaInputFormat.java
        • /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/util/DumpChunks.java
        • /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/util/Filter.java
        • /incubator/chukwa/trunk/src/test/java/org/apache/hadoop/chukwa/extraction/demux/processor/mapper/TestTsProcessor.java
        • /incubator/chukwa/trunk/src/test/java/org/apache/hadoop/chukwa/inputtools/TestInputFormat.java
        • /incubator/chukwa/trunk/src/test/java/org/apache/hadoop/chukwa/util/TestDumpChunks.java
        • /incubator/chukwa/trunk/src/test/java/org/apache/hadoop/chukwa/util/TestFilter.java
        Show
        hudson Hudson added a comment - Integrated in Chukwa-trunk #460 (See https://builds.apache.org/job/Chukwa-trunk/460/ ) CHUKWA-642 . Added regular expression validation. (Eric Spishak via Eric Yang) (Revision 1411833) CHUKWA-642 . Added regular expression validation. (Eric Spishak via Eric Yang) (Revision 1411813) Result = SUCCESS eyang : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1411833 Files : /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/util/RegexUtil.java eyang : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1411813 Files : /incubator/chukwa/trunk/CHANGES.txt /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/database/DataExpiration.java /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/database/TableCreator.java /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/datacollection/writer/SocketTeeWriter.java /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/dataloader/MetricDataLoader.java /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/extraction/demux/processor/mapper/TsProcessor.java /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/inputtools/ChukwaInputFormat.java /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/util/DumpChunks.java /incubator/chukwa/trunk/src/main/java/org/apache/hadoop/chukwa/util/Filter.java /incubator/chukwa/trunk/src/test/java/org/apache/hadoop/chukwa/extraction/demux/processor/mapper/TestTsProcessor.java /incubator/chukwa/trunk/src/test/java/org/apache/hadoop/chukwa/inputtools/TestInputFormat.java /incubator/chukwa/trunk/src/test/java/org/apache/hadoop/chukwa/util/TestDumpChunks.java /incubator/chukwa/trunk/src/test/java/org/apache/hadoop/chukwa/util/TestFilter.java

          People

          • Assignee:
            espishak Eric Spishak
            Reporter:
            espishak Eric Spishak
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development