[FIXED JENKINS-25511] [JENKINS-17309] Reverted XML escaping of message.

Revert of fix for JENKINS-17309. Previous solution was too aggressive, it makes sense to escape only messages from parsers that parse non-XML files. XML parsers should not escape entities at all. AbstractAnnotation never escapes entities now, only GCC parser escapes and invokes constructor with escaped message.
jenkinsci · Nov 16, 2014 · 7af227e · 7af227e
1 parent d9427bb
commit 7af227e
Show file tree

Hide file tree

Showing 7 changed files with 49 additions and 21 deletions.
diff --git a/pom.xml b/pom.xml
@@ -49,7 +49,7 @@
     <dependency>
       <groupId>org.jvnet.hudson.plugins</groupId>
       <artifactId>analysis-core</artifactId>
-      <version>1.65</version>
+      <version>1.66-SNAPSHOT</version>
     </dependency>
     <dependency>
       <groupId>org.jvnet.hudson.plugins</groupId>

diff --git a/src/main/java/hudson/plugins/warnings/parser/AbstractWarningsParser.java b/src/main/java/hudson/plugins/warnings/parser/AbstractWarningsParser.java
@@ -7,6 +7,7 @@
 import java.util.Locale;
 
 import org.apache.commons.lang.StringUtils;
+import org.apache.commons.lang3.StringEscapeUtils;
 import org.jvnet.localizer.Localizable;
 
 import hudson.ExtensionPoint;
@@ -319,5 +320,16 @@ public static int convertLineNumber(final String lineNumber) {
         }
         return 0;
     }
+
+    /**
+     * Escapes the characters in a {@code String} using XML entities.
+     *
+     * @param text the text to escape
+     * @return the escaped text
+     * @see StringEscapeUtils#escapeXml11
+     */
+    protected String escapeXml(final String text) {
+        return StringEscapeUtils.escapeXml10(text);
+    }
 }
 
diff --git a/src/main/java/hudson/plugins/warnings/parser/GccParser.java b/src/main/java/hudson/plugins/warnings/parser/GccParser.java
@@ -5,7 +5,6 @@
 import org.apache.commons.lang.StringUtils;
 
 import hudson.Extension;
-
 import hudson.plugins.analysis.util.model.Priority;
 
 /**
@@ -58,13 +57,14 @@ else if (StringUtils.isNotBlank(matcher.group(4))) {
             if (matcher.group(4).contains("instantiated from here")) {
                 return FALSE_POSITIVE;
             }
-            return createWarning(fileName, getLineNumber(matcher.group(2)), GCC_ERROR, matcher.group(4), Priority.HIGH);
+            return createWarning(fileName, getLineNumber(matcher.group(2)), GCC_ERROR, escapeXml(matcher.group(4)), Priority.HIGH);
         }
         else {
-            return createWarning(fileName, 0, GCC_ERROR, matcher.group(5), Priority.HIGH);
+            return createWarning(fileName, 0, GCC_ERROR, escapeXml(matcher.group(5)), Priority.HIGH);
         }
         String category = "GCC " + matcher.group(3);
-        return createWarning(fileName, getLineNumber(matcher.group(2)), category, matcher.group(6), priority);
+        return createWarning(fileName, getLineNumber(matcher.group(2)), category, escapeXml(matcher.group(6)), priority);
     }
+
 }
 
diff --git a/src/test/java/hudson/plugins/warnings/parser/GccParserTest.java b/src/test/java/hudson/plugins/warnings/parser/GccParserTest.java
@@ -1,13 +1,13 @@
 package hudson.plugins.warnings.parser;
 
-import static org.junit.Assert.*;
-
 import java.io.IOException;
 import java.util.Collection;
 import java.util.Iterator;
 
 import org.junit.Test;
 
+import static org.junit.Assert.*;
+
 import hudson.plugins.analysis.util.model.FileAnnotation;
 import hudson.plugins.analysis.util.model.Priority;
 
@@ -19,6 +19,21 @@ public class GccParserTest extends ParserTester {
     private static final String GCC_ERROR = GccParser.GCC_ERROR;
     private static final String GCC_WARNING = "GCC warning";
 
+    /**
+     * Verifies that the message contains escaped XML characters.
+     *
+     * @see <a href="http://issues.jenkins-ci.org/browse/JENKINS-17309">Issue 17309</a>
+     */
+    @Test
+    public void issue17309() throws IOException {
+        Collection<FileAnnotation> warnings = new GccParser().parse(openFile("issue17309.txt"));
+
+        assertEquals(WRONG_NUMBER_OF_WARNINGS_DETECTED, 1, warnings.size());
+        FileAnnotation annotation = warnings.iterator().next();
+        checkWarning(annotation, 4, "dereferencing pointer &apos;&lt;anonymous&gt;&apos; does break strict-aliasing rules",
+                "foo.cc", TYPE, GCC_ERROR, Priority.HIGH);
+    }
+
     /**
      * Parses a file with one warning that are started by ant.
      *
@@ -53,7 +68,7 @@ public void testWarningsParser() throws IOException {
         FileAnnotation annotation = iterator.next();
         checkWarning(annotation,
                 451,
-                "`void yyunput(int, char*)' defined but not used",
+                "`void yyunput(int, char*)&apos; defined but not used",
                 "testhist.l",
                 TYPE, GCC_WARNING, Priority.NORMAL);
         annotation = iterator.next();
@@ -71,7 +86,7 @@ public void testWarningsParser() throws IOException {
         annotation = iterator.next();
         checkWarning(annotation,
                 0,
-                "undefined reference to 'missing_symbol'",
+                "undefined reference to &apos;missing_symbol&apos;",
                 "foo.so",
                 TYPE, GCC_ERROR, Priority.HIGH);
         annotation = iterator.next();
@@ -120,7 +135,7 @@ public void issue3897and3898() throws IOException {
                 TYPE, GccParser.GCC_ERROR, Priority.HIGH);
         checkWarning(iterator.next(),
                 233,
-                "undefined reference to `MyInterface::getValue() const'",
+                "undefined reference to `MyInterface::getValue() const&apos;",
                 "/dir1/dir3/file.cpp",
                 TYPE, GccParser.GCC_ERROR, Priority.HIGH);
         checkWarning(iterator.next(),
@@ -145,12 +160,12 @@ public void issue4712() throws IOException {
         Iterator<FileAnnotation> iterator = warnings.iterator();
         checkWarning(iterator.next(),
                 352,
-                "'s2.mepSector2::lubrications' may be used",
+                "&apos;s2.mepSector2::lubrications&apos; may be used",
                 "main/mep.cpp",
                 TYPE, GCC_WARNING, Priority.NORMAL);
         checkWarning(iterator.next(),
                 1477,
-                "'s2.mepSector2::lubrications' was declared here",
+                "&apos;s2.mepSector2::lubrications&apos; was declared here",
                 "main/mep.cpp",
                 TYPE, "GCC note", Priority.LOW);
     }
@@ -224,22 +239,22 @@ public void issue4274() throws IOException {
         Iterator<FileAnnotation> iterator = warnings.iterator();
         checkWarning(iterator.next(),
                 638,
-                "local declaration of \"command\" hides instance variable",
+                "local declaration of &quot;command&quot; hides instance variable",
                 "folder1/file1.m",
                 TYPE, GCC_WARNING, Priority.NORMAL);
         checkWarning(iterator.next(),
                 640,
-                "instance variable \"command\" accessed in class method",
+                "instance variable &quot;command&quot; accessed in class method",
                 "folder1/file1.m",
                 TYPE, GCC_WARNING, Priority.NORMAL);
         checkWarning(iterator.next(),
                 47,
-                "\"oldGeb\" might be used uninitialized in this function",
+                "&quot;oldGeb&quot; might be used uninitialized in this function",
                 "file1.m",
                 TYPE, GCC_WARNING, Priority.NORMAL);
         checkWarning(iterator.next(),
                 640,
-                "local declaration of \"command\" hides instance variable",
+                "local declaration of &quot;command&quot; hides instance variable",
                 "file1.m",
                 TYPE, GCC_WARNING, Priority.NORMAL);
     }

diff --git a/src/test/java/hudson/plugins/warnings/parser/ParserTester.java b/src/test/java/hudson/plugins/warnings/parser/ParserTester.java
@@ -1,13 +1,12 @@
 package hudson.plugins.warnings.parser;
 
-import static org.junit.Assert.*;
-
 import java.io.InputStreamReader;
 import java.io.Reader;
 import java.io.UnsupportedEncodingException;
 
 import org.apache.commons.io.input.BOMInputStream;
-import org.apache.commons.lang.StringEscapeUtils;
+
+import static org.junit.Assert.*;
 
 import hudson.plugins.analysis.util.model.FileAnnotation;
 import hudson.plugins.analysis.util.model.Priority;
@@ -41,7 +40,7 @@ protected void checkWarning(final FileAnnotation annotation, final int lineNumbe
         assertEquals("Wrong number of ranges detected: ", 1, warning.getLineRanges().size());
         assertEquals("Wrong ranges start detected: ", lineNumber, warning.getLineRanges().iterator().next().getStart());
         assertEquals("Wrong ranges end detected: ", lineNumber, warning.getLineRanges().iterator().next().getEnd());
-        assertEquals("Wrong message detected: ", StringEscapeUtils.escapeXml(message), warning.getMessage());
+        assertEquals("Wrong message detected: ", message, warning.getMessage());
         assertEquals("Wrong filename detected: ", fileName, warning.getFileName());
     }
 

diff --git a/src/test/resources/hudson/plugins/warnings/parser/issue17309.txt b/src/test/resources/hudson/plugins/warnings/parser/issue17309.txt
@@ -0,0 +1,2 @@
+foo.cc:4:39: error: dereferencing pointer '<anonymous>' does break strict-aliasing rules
+
diff --git a/warnings.iml b/warnings.iml
@@ -21,7 +21,7 @@
     </content>
     <orderEntry type="inheritedJdk" />
     <orderEntry type="sourceFolder" forTests="false" />
-    <orderEntry type="library" name="Maven: org.jvnet.hudson.plugins:analysis-core:1.65" level="project" />
+    <orderEntry type="module" module-name="analysis-core" />
     <orderEntry type="library" name="Maven: de.java2html:java2html:5.0" level="project" />
     <orderEntry type="library" name="Maven: org.apache.commons:commons-lang3:3.3.2" level="project" />
     <orderEntry type="library" name="Maven: xerces:xercesImpl:2.11.0" level="project" />