[FIXED JENKINS-18337] Must use xmlEscape for freeform text fields in …

…fingerprint XML. Conflicts: changelog.html
jenkinsci · Sep 9, 2013 · c80cd42 · c80cd42
1 parent b01861a
commit c80cd42
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 8 deletions.
diff --git a/core/src/main/java/hudson/model/Fingerprint.java b/core/src/main/java/hudson/model/Fingerprint.java
@@ -66,6 +66,8 @@
 import java.util.TreeMap;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import javax.annotation.CheckForNull;
+import org.xmlpull.v1.XmlPullParserException;
 
 /**
  * A file being tracked by Jenkins.
@@ -879,7 +881,7 @@ void save(File file) throws IOException {
                 if (original != null) {
                     w.println("  <original>");
                     w.print("    <name>");
-                    w.print(original.name);
+                    w.print(Util.xmlEscape(original.name));
                     w.println("</name>");
                     w.print("    <number>");
                     w.print(original.number);
@@ -890,13 +892,13 @@ void save(File file) throws IOException {
                 w.print(Util.toHexString(md5sum));
                 w.println("</md5sum>");
                 w.print("  <fileName>");
-                w.print(fileName);
+                w.print(Util.xmlEscape(fileName));
                 w.println("</fileName>");
                 w.println("  <usages>");
                 for (Map.Entry<String,RangeSet> e : usages.entrySet()) {
                     w.println("    <entry>");
                     w.print("      <string>");
-                    w.print(e.getKey());
+                    w.print(Util.xmlEscape(e.getKey()));
                     w.println("</string>");
                     w.print("      <ranges>");
                     w.print(RangeSet.ConverterImpl.serialize(e.getValue()));
@@ -965,10 +967,10 @@ private static File getFingerprintFile(byte[] md5sum) {
     /**
      * Loads a {@link Fingerprint} from a file in the image.
      */
-    /*package*/ static Fingerprint load(byte[] md5sum) throws IOException {
+    /*package*/ static @CheckForNull Fingerprint load(byte[] md5sum) throws IOException {
         return load(getFingerprintFile(md5sum));
     }
-    /*package*/ static Fingerprint load(File file) throws IOException {
+    /*package*/ static @CheckForNull Fingerprint load(File file) throws IOException {
         XmlFile configFile = getConfigFile(file);
         if(!configFile.exists())
             return null;
@@ -994,14 +996,31 @@ private static File getFingerprintFile(byte[] md5sum) {
                 // generally we don't want to wipe out user data just because we can't load it,
                 // but if the file size is 0, which is what's reported in HUDSON-2012, then it seems
                 // like recovering it silently by deleting the file is not a bad idea.
-                logger.log(Level.WARNING, "Size zero fingerprint. Disk corruption? "+configFile,e);
+                logger.log(Level.WARNING, "Size zero fingerprint. Disk corruption? {0}", configFile);
+                file.delete();
+                return null;
+            }
+            String parseError = messageOfXmlPullParserException(e);
+            if (parseError != null) {
+                logger.log(Level.WARNING, "Malformed XML in {0}: {1}", new Object[] {configFile, parseError});
                 file.delete();
                 return null;
             }
             logger.log(Level.WARNING, "Failed to load "+configFile,e);
             throw e;
         }
     }
+    private static String messageOfXmlPullParserException(Throwable t) {
+        if (t instanceof XmlPullParserException) {
+            return t.getMessage();
+        }
+        Throwable t2 = t.getCause();
+        if (t2 != null) {
+            return messageOfXmlPullParserException(t2);
+        } else {
+            return null;
+        }
+    }
 
     @Override public String toString() {
         return "Fingerprint[original=" + original + ",hash=" + getHashString() + ",fileName=" + fileName + ",timestamp=" + DATE_CONVERTER.toString(timestamp) + ",usages=" + new TreeMap<String,RangeSet>(usages) + ",facets=" + facets + "]";

diff --git a/core/src/main/java/hudson/model/FingerprintCleanupThread.java b/core/src/main/java/hudson/model/FingerprintCleanupThread.java
@@ -100,7 +100,7 @@ private void deleteIfEmpty(File dir) {
     private boolean check(File fingerprintFile) {
         try {
             Fingerprint fp = Fingerprint.load(fingerprintFile);
-            if(!fp.isAlive()) {
+            if (fp == null || !fp.isAlive()) {
                 fingerprintFile.delete();
                 return true;
             }

diff --git a/core/src/test/java/hudson/model/FingerprintTest.java b/core/src/test/java/hudson/model/FingerprintTest.java
@@ -135,7 +135,7 @@ public class FingerprintTest {
     }
 
     @Test public void roundTrip() throws Exception {
-        Fingerprint f = new Fingerprint(new Fingerprint.BuildPtr("foo", 13), "stuff.jar", SOME_MD5);
+        Fingerprint f = new Fingerprint(new Fingerprint.BuildPtr("foo", 13), "stuff&more.jar", SOME_MD5);
         f.addWithoutSaving("some", 1);
         f.addWithoutSaving("some", 2);
         f.addWithoutSaving("some", 3);
@@ -144,6 +144,7 @@ public class FingerprintTest {
         File xml = new File(new File(tmp.getRoot(), "dir"), "fp.xml");
         f.save(xml);
         Fingerprint f2 = Fingerprint.load(xml);
+        assertNotNull(f2);
         assertEquals(f.toString(), f2.toString());
         f.facets.setOwner(Saveable.NOOP);
         f.facets.add(new TestFacet(f, 123, "val"));