From 31a2d09c9c770e80be43685cb4d6ca2eff21b75d Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 18 Dec 2017 15:35:02 -0500 Subject: [PATCH] Config: Rewrite subsection and value escaping and parsing Previously, Config was using the same method for both escaping and parsing subsection names and config values. The goal was presumably code savings, but unfortunately, these two pieces of the git config format are simply different. In git v2.15.1, Documentation/config.txt says the following about subsection names: "Subsection names are case sensitive and can contain any characters except newline (doublequote `"` and backslash can be included by escaping them as `\"` and `\\`, respectively). Section headers cannot span multiple lines. Variables may belong directly to a section or to a given subsection." And, later in the same documentation section, about values: "A line that defines a value can be continued to the next line by ending it with a `\`; the backquote and the end-of-line are stripped. Leading whitespaces after 'name =', the remainder of the line after the first comment character '#' or ';', and trailing whitespaces of the line are discarded unless they are enclosed in double quotes. Internal whitespaces within the value are retained verbatim. Inside double quotes, double quote `"` and backslash `\` characters must be escaped: use `\"` for `"` and `\\` for `\`. The following escape sequences (beside `\"` and `\\`) are recognized: `\n` for newline character (NL), `\t` for horizontal tabulation (HT, TAB) and `\b` for backspace (BS). Other char escape sequences (including octal escape sequences) are invalid." The main important differences are that subsection names have a limited set of supported escape sequences, and do not support newlines at all, either escaped or unescaped. Arguably, it would be easy to support escaped newlines, but C git simply does not: $ git config -f foo.config $'foo.bar\nbaz.quux' value error: invalid key (newline): foo.bar baz.quux I468106ac was an attempt to fix one bug in escapeValue, around leading whitespace, without having to rewrite the whole escaping/parsing code. Unfortunately, because escapeValue was used for escaping subsection names as well, this made it possible to write invalid config files, any time Config#toText is called with a subsection name with trailing whitespace, like {foo }. Rather than pile hacks on top of hacks, fix it for real by largely rewriting the escaping and parsing code. In addition to fixing escape sequences, fix (and write tests for) a few more issues in the old implementation: * Now that we can properly parse it, always emit newlines as "\n" from escapeValue, rather than the weird (but still supported) syntax with a non-quoted trailing literal "\n\" before the newline. In addition to producing more readable output and matching the behavior of C git, this makes the escaping code much simpler. * Disallow '\0' entirely within both subsection names and values, since due to Unix command line argument conventions it is impossible to pass such values to "git config". * Properly preserve intra-value whitespace when parsing, rather than collapsing it all to a single space. Change-Id: I304f626b9d0ad1592c4e4e449a11b136c0f8b3e3 --- .../storage/file/T0003_BasicTest.java | 16 +- .../tst/org/eclipse/jgit/lib/ConfigTest.java | 217 ++++++++++++++---- .../eclipse/jgit/internal/JGitText.properties | 3 + .../org/eclipse/jgit/internal/JGitText.java | 3 + .../src/org/eclipse/jgit/lib/Config.java | 191 +++++++++------ 5 files changed, 307 insertions(+), 123 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/T0003_BasicTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/T0003_BasicTest.java index a3a435674..ab86bc2e2 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/T0003_BasicTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/T0003_BasicTest.java @@ -56,7 +56,6 @@ import static org.junit.Assert.fail; import java.io.File; import java.io.FileInputStream; -import java.io.FileReader; import java.io.IOException; import java.io.UnsupportedEncodingException; @@ -83,6 +82,7 @@ import org.eclipse.jgit.storage.file.FileBasedConfig; import org.eclipse.jgit.storage.file.FileRepositoryBuilder; import org.eclipse.jgit.test.resources.SampleDataRepositoryTestCase; import org.eclipse.jgit.util.FileUtils; +import org.eclipse.jgit.util.IO; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -361,11 +361,15 @@ public class T0003_BasicTest extends SampleDataRepositoryTestCase { assertEquals("a many line\ncomment\n to test", c.getString("user", null, "defaultCheckInComment")); c.save(); - final FileReader fr = new FileReader(cfg); - final char[] cbuf = new char[configStr.length()]; - fr.read(cbuf); - fr.close(); - assertEquals(configStr, new String(cbuf)); + + // Saving normalizes out the weird "\\n\\\n" to a single escaped newline, + // and quotes the whole string. + final String expectedStr = " [core];comment\n\tfilemode = yes\n" + + "[user]\n" + + " email = A U Thor # Just an example...\n" + + " name = \"A Thor \\\\ \\\"\\t \"\n" + + " defaultCheckInComment = \"a many line\\ncomment\\n to test\"\n"; + assertEquals(expectedStr, new String(IO.readFully(cfg), Constants.CHARSET)); } @Test diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java index 712e9df44..44714faa7 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java @@ -89,7 +89,10 @@ import org.junit.rules.TemporaryFolder; /** * Test reading of git config */ +@SuppressWarnings("boxing") public class ConfigTest { + // A non-ASCII whitespace character: U+2002 EN QUAD. + private static final char WS = '\u2002'; @Rule public ExpectedException expectedEx = ExpectedException.none(); @@ -666,28 +669,6 @@ public class ConfigTest { assertTrue("Subsection should contain \"B\"", names.contains("B")); } - @Test - public void testQuotingForSubSectionNames() { - String resultPattern = "[testsection \"{0}\"]\n\ttestname = testvalue\n"; - String result; - - Config config = new Config(); - config.setString("testsection", "testsubsection", "testname", - "testvalue"); - - result = MessageFormat.format(resultPattern, "testsubsection"); - assertEquals(result, config.toText()); - config.clear(); - - config.setString("testsection", "#quotable", "testname", "testvalue"); - result = MessageFormat.format(resultPattern, "#quotable"); - assertEquals(result, config.toText()); - config.clear(); - - config.setString("testsection", "with\"quote", "testname", "testvalue"); - result = MessageFormat.format(resultPattern, "with\\\"quote"); - assertEquals(result, config.toText()); - } @Test public void testNoFinalNewline() throws ConfigInvalidException { @@ -956,53 +937,187 @@ public class ConfigTest { @Test public void testEscapeSpacesOnly() throws ConfigInvalidException { + // Empty string is read back as null, so this doesn't round-trip. assertEquals("", Config.escapeValue("")); - assertEquals("\" \"", Config.escapeValue(" ")); - assertEquals("\" \"", Config.escapeValue(" ")); - assertParseRoundTrip(" "); - assertParseRoundTrip(" "); + assertValueRoundTrip(" ", "\" \""); + assertValueRoundTrip(" ", "\" \""); } @Test public void testEscapeLeadingSpace() throws ConfigInvalidException { - assertEquals("x", Config.escapeValue("x")); - assertEquals("\" x\"", Config.escapeValue(" x")); - assertEquals("\" x\"", Config.escapeValue(" x")); - - assertParseRoundTrip("x"); - assertParseRoundTrip(" x"); - assertParseRoundTrip(" x"); + assertValueRoundTrip("x", "x"); + assertValueRoundTrip(" x", "\" x\""); + assertValueRoundTrip(" x", "\" x\""); } @Test public void testEscapeTrailingSpace() throws ConfigInvalidException { - assertEquals("x", Config.escapeValue("x")); - assertEquals("\"x \"", Config.escapeValue("x ")); - assertEquals("x\" \"", Config.escapeValue("x ")); - - assertParseRoundTrip("x"); - assertParseRoundTrip("x "); - assertParseRoundTrip("x "); + assertValueRoundTrip("x", "x"); + assertValueRoundTrip("x ","\"x \""); + assertValueRoundTrip("x ","\"x \""); } @Test public void testEscapeLeadingAndTrailingSpace() throws ConfigInvalidException { - assertEquals("\" x \"", Config.escapeValue(" x ")); - assertEquals("\" x \"", Config.escapeValue(" x ")); - assertEquals("\" x \"", Config.escapeValue(" x ")); - assertEquals("\" x \"", Config.escapeValue(" x ")); + assertValueRoundTrip(" x ", "\" x \""); + assertValueRoundTrip(" x ", "\" x \""); + assertValueRoundTrip(" x ", "\" x \""); + assertValueRoundTrip(" x ", "\" x \""); + } + + @Test + public void testNoEscapeInternalSpaces() throws ConfigInvalidException { + assertValueRoundTrip("x y"); + assertValueRoundTrip("x y"); + assertValueRoundTrip("x y"); + assertValueRoundTrip("x y z"); + assertValueRoundTrip("x " + WS + " y"); + } + + @Test + public void testEscapeSpecialCharacters() throws ConfigInvalidException { + assertValueRoundTrip("x#y", "\"x#y\""); + assertValueRoundTrip("x;y", "\"x;y\""); + assertValueRoundTrip("x\\y", "\"x\\\\y\""); + assertValueRoundTrip("x\"y", "\"x\\\"y\""); + assertValueRoundTrip("x\ny", "\"x\\ny\""); + assertValueRoundTrip("x\ty", "\"x\\ty\""); + assertValueRoundTrip("x\by", "\"x\\by\""); + } + + @Test + public void testEscapeValueInvalidCharacters() { + assertIllegalArgumentException(() -> Config.escapeSubsection("x\0y")); + } + + @Test + public void testEscapeSubsectionInvalidCharacters() { + assertIllegalArgumentException(() -> Config.escapeSubsection("x\ny")); + assertIllegalArgumentException(() -> Config.escapeSubsection("x\0y")); + } + + @Test + public void testParseMultipleQuotedRegions() throws ConfigInvalidException { + assertEquals("b a z; \n", parseEscapedValue("b\" a\"\" z; \\n\"")); + } + + @Test + public void testParseComments() throws ConfigInvalidException { + assertEquals("baz", parseEscapedValue("baz; comment")); + assertEquals("baz", parseEscapedValue("baz# comment")); + assertEquals("baz", parseEscapedValue("baz ; comment")); + assertEquals("baz", parseEscapedValue("baz # comment")); + + assertEquals("baz", parseEscapedValue("baz ; comment")); + assertEquals("baz", parseEscapedValue("baz # comment")); + assertEquals("baz", parseEscapedValue("baz " + WS + " ; comment")); + assertEquals("baz", parseEscapedValue("baz " + WS + " # comment")); + } + + @Test + public void testEscapeSubsection() throws ConfigInvalidException { + assertSubsectionRoundTrip("", "\"\""); + assertSubsectionRoundTrip("x", "\"x\""); + assertSubsectionRoundTrip(" x", "\" x\""); + assertSubsectionRoundTrip("x ", "\"x \""); + assertSubsectionRoundTrip(" x ", "\" x \""); + assertSubsectionRoundTrip("x y", "\"x y\""); + assertSubsectionRoundTrip("x y", "\"x y\""); + assertSubsectionRoundTrip("x\\y", "\"x\\\\y\""); + assertSubsectionRoundTrip("x\"y", "\"x\\\"y\""); + + // Unlike for values, \b and \t are not escaped. + assertSubsectionRoundTrip("x\by", "\"x\by\""); + assertSubsectionRoundTrip("x\ty", "\"x\ty\""); + } + + @Test + public void testParseInvalidValues() { + assertInvalidValue(JGitText.get().newlineInQuotesNotAllowed, "x\"\n\"y"); + assertInvalidValue(JGitText.get().endOfFileInEscape, "x\\"); + assertInvalidValue( + MessageFormat.format(JGitText.get().badEscape, 'q'), "x\\q"); + } + + @Test + public void testParseInvalidSubsections() { + assertInvalidSubsection( + JGitText.get().newlineInQuotesNotAllowed, "\"x\ny\""); + assertInvalidSubsection( + MessageFormat.format(JGitText.get().badEscape, 'q'), "\"x\\q\""); + + // Unlike for values, \b, \n, and \t are not valid escape sequences. + assertInvalidSubsection( + MessageFormat.format(JGitText.get().badEscape, 'b'), "\"x\\b\""); + assertInvalidSubsection( + MessageFormat.format(JGitText.get().badEscape, 'n'), "\"x\\n\""); + assertInvalidSubsection( + MessageFormat.format(JGitText.get().badEscape, 't'), "\"x\\t\""); + } + + private static void assertValueRoundTrip(String value) + throws ConfigInvalidException { + assertValueRoundTrip(value, value); + } + + private static void assertValueRoundTrip(String value, String expectedEscaped) + throws ConfigInvalidException { + String escaped = Config.escapeValue(value); + assertEquals("escape failed;", expectedEscaped, escaped); + assertEquals("parse failed;", value, parseEscapedValue(escaped)); + } + + private static String parseEscapedValue(String escapedValue) + throws ConfigInvalidException { + String text = "[foo]\nbar=" + escapedValue; + Config c = parse(text); + return c.getString("foo", null, "bar"); + } + + private static void assertInvalidValue(String expectedMessage, + String escapedValue) { + try { + parseEscapedValue(escapedValue); + fail("expected ConfigInvalidException"); + } catch (ConfigInvalidException e) { + assertEquals(expectedMessage, e.getMessage()); + } + } - assertParseRoundTrip(" x "); - assertParseRoundTrip(" x "); - assertParseRoundTrip(" x "); - assertParseRoundTrip(" x "); + private static void assertSubsectionRoundTrip(String subsection, + String expectedEscaped) throws ConfigInvalidException { + String escaped = Config.escapeSubsection(subsection); + assertEquals("escape failed;", expectedEscaped, escaped); + assertEquals("parse failed;", subsection, parseEscapedSubsection(escaped)); } - private static void assertParseRoundTrip(String value) + private static String parseEscapedSubsection(String escapedSubsection) throws ConfigInvalidException { - Config c = parse("[foo]\nbar = " + Config.escapeValue(value)); - assertEquals(value, c.getString("foo", null, "bar")); + String text = "[foo " + escapedSubsection + "]\nbar = value"; + Config c = parse(text); + Set subsections = c.getSubsections("foo"); + assertEquals("only one section", 1, subsections.size()); + return subsections.iterator().next(); + } + + private static void assertIllegalArgumentException(Runnable r) { + try { + r.run(); + fail("expected IllegalArgumentException"); + } catch (IllegalArgumentException e) { + // Expected. + } + } + + private static void assertInvalidSubsection(String expectedMessage, + String escapedSubsection) { + try { + parseEscapedSubsection(escapedSubsection); + fail("expected ConfigInvalidException"); + } catch (ConfigInvalidException e) { + assertEquals(expectedMessage, e.getMessage()); + } } } diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index 85d87349f..dacdd0542 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -130,6 +130,9 @@ commitMessageNotSpecified=commit message not specified commitOnRepoWithoutHEADCurrentlyNotSupported=Commit on repo without HEAD currently not supported commitAmendOnInitialNotPossible=Amending is not possible on initial commit. compressingObjects=Compressing objects +configSubsectionContainsNewline=config subsection name contains newline +configSubsectionContainsNullByte=config subsection name contains byte 0x00 +configValueContainsNullByte=config value contains byte 0x00 connectionFailed=connection failed connectionTimeOut=Connection time out: {0} contextMustBeNonNegative=context must be >= 0 diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index 35c8076b3..836312276 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -191,6 +191,9 @@ public class JGitText extends TranslationBundle { /***/ public String commitOnRepoWithoutHEADCurrentlyNotSupported; /***/ public String commitAmendOnInitialNotPossible; /***/ public String compressingObjects; + /***/ public String configSubsectionContainsNewline; + /***/ public String configSubsectionContainsNullByte; + /***/ public String configValueContainsNullByte; /***/ public String connectionFailed; /***/ public String connectionTimeOut; /***/ public String contextMustBeNonNegative; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java index 3e28184b5..b98586cfd 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java @@ -144,81 +144,91 @@ public class Config { * the value to escape * @return the escaped value */ - static String escapeValue(final String x) { + static String escapeValue(String x) { if (x.isEmpty()) { return ""; //$NON-NLS-1$ } - boolean inquote = false; - int lineStart = 0; - final StringBuilder r = new StringBuilder(x.length()); + + boolean needQuote = x.charAt(0) == ' ' || x.charAt(x.length() - 1) == ' '; + StringBuilder r = new StringBuilder(x.length()); for (int k = 0; k < x.length(); k++) { - final char c = x.charAt(k); + char c = x.charAt(k); + boolean thisCharNeedsQuote = true; + + // git-config(1) lists the limited set of supported escape sequences. switch (c) { + case '\0': + // Unix command line calling convention cannot pass a '\0' as an + // argument, so there is no equivalent way in C git to store a null byte + // in a config value. + throw new IllegalArgumentException( + JGitText.get().configValueContainsNullByte); + case '\n': - if (inquote) { - r.append('"'); - inquote = false; - } - r.append("\\n\\\n"); //$NON-NLS-1$ - lineStart = r.length(); + r.append('\\').append('n'); break; case '\t': - r.append("\\t"); //$NON-NLS-1$ + r.append('\\').append('t'); break; case '\b': - r.append("\\b"); //$NON-NLS-1$ + r.append('\\').append('b'); break; case '\\': - r.append("\\\\"); //$NON-NLS-1$ - break; - case '"': - r.append("\\\""); //$NON-NLS-1$ + r.append('\\').append(c); break; - case ';': case '#': - if (!inquote) { - r.insert(lineStart, '"'); - inquote = true; - } + case ';': r.append(c); break; - case ' ': - if (!inquote && (r.length() == 0 || r.charAt(r.length() - 1) == ' ')) { - r.insert(lineStart, '"'); - inquote = true; - } - r.append(' '); - break; - default: + thisCharNeedsQuote = false; r.append(c); break; } + needQuote |= thisCharNeedsQuote; } - if (!inquote) { - // Ensure any trailing whitespace is quoted. - int s = x.length(); - while (s > 0 && x.charAt(s - 1) == ' ') { - s--; - } - if (s != x.length()) { - // Can't insert at lineStart since there may be intervening quotes. - r.insert(s, '"'); - inquote = true; - } + return needQuote ? '"' + r.toString() + '"' : r.toString(); + } + + static String escapeSubsection(String x) { + if (x.isEmpty()) { + return "\"\""; //$NON-NLS-1$ } - if (inquote) { - r.append('"'); + StringBuilder r = new StringBuilder(x.length() + 2).append('"'); + for (int k = 0; k < x.length(); k++) { + char c = x.charAt(k); + + // git-config(1) lists the limited set of supported escape sequences + // (which is even more limited for subsection names than for values). + switch (c) { + case '\0': + throw new IllegalArgumentException( + JGitText.get().configSubsectionContainsNullByte); + + case '\n': + throw new IllegalArgumentException( + JGitText.get().configSubsectionContainsNewline); + + case '\\': + case '"': + r.append('\\').append(c); + break; + + default: + r.append(c); + break; + } } - return r.toString(); + + return r.append('"').toString(); } /** @@ -1068,7 +1078,7 @@ public class Config { e.section = readSectionName(in); input = in.read(); if ('"' == input) { - e.subsection = readValue(in, true, '"'); + e.subsection = readSubsectionName(in); input = in.read(); } if (']' != input) @@ -1085,7 +1095,7 @@ public class Config { e.name = e.name.substring(0, e.name.length() - 1); e.value = MAGIC_EMPTY_VALUE; } else - e.value = readValue(in, false, -1); + e.value = readValue(in); if (e.section.equals("include")) { //$NON-NLS-1$ addIncludedConfig(newEntries, e, depth); @@ -1252,10 +1262,9 @@ public class Config { return name.toString(); } - private static String readValue(final StringReader in, boolean quote, - final int eol) throws ConfigInvalidException { - final StringBuilder value = new StringBuilder(); - boolean space = false; + private static String readSubsectionName(StringReader in) + throws ConfigInvalidException { + StringBuilder r = new StringBuilder(); for (;;) { int c = in.read(); if (c < 0) { @@ -1263,30 +1272,80 @@ public class Config { } if ('\n' == c) { - if (quote) - throw new ConfigInvalidException(JGitText.get().newlineInQuotesNotAllowed); + throw new ConfigInvalidException( + JGitText.get().newlineInQuotesNotAllowed); + } + if ('\\' == c) { + c = in.read(); + switch (c) { + case -1: + throw new ConfigInvalidException(JGitText.get().endOfFileInEscape); + + case '\\': + case '"': + r.append((char) c); + continue; + + default: + throw new ConfigInvalidException(MessageFormat.format( + JGitText.get().badEscape, + Character.valueOf(((char) c)))); + } + } + if ('"' == c) { + break; + } + + r.append((char) c); + } + return r.toString(); + } + + private static String readValue(final StringReader in) + throws ConfigInvalidException { + StringBuilder value = new StringBuilder(); + StringBuilder trailingSpaces = null; + boolean quote = false; + boolean inLeadingSpace = true; + + for (;;) { + int c = in.read(); + if (c < 0) { + break; + } + if ('\n' == c) { + if (quote) { + throw new ConfigInvalidException( + JGitText.get().newlineInQuotesNotAllowed); + } in.reset(); break; } - if (eol == c) + if (!quote && (';' == c || '#' == c)) { + if (trailingSpaces != null) { + trailingSpaces.setLength(0); + } + in.reset(); break; + } - if (!quote) { - if (Character.isWhitespace((char) c)) { - space = true; + char cc = (char) c; + if (Character.isWhitespace(cc)) { + if (inLeadingSpace) { continue; } - if (';' == c || '#' == c) { - in.reset(); - break; + if (trailingSpaces == null) { + trailingSpaces = new StringBuilder(); + } + trailingSpaces.append(cc); + continue; + } else { + inLeadingSpace = false; + if (trailingSpaces != null) { + value.append(trailingSpaces); + trailingSpaces.setLength(0); } - } - - if (space) { - if (value.length() > 0) - value.append(' '); - space = false; } if ('\\' == c) { @@ -1323,7 +1382,7 @@ public class Config { continue; } - value.append((char) c); + value.append(cc); } return value.length() > 0 ? value.toString() : null; }