Browse Source

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
stable-4.10
Dave Borowitz 7 years ago
parent
commit
31a2d09c9c
  1. 16
      org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/T0003_BasicTest.java
  2. 217
      org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java
  3. 3
      org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
  4. 3
      org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
  5. 187
      org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java

16
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.File;
import java.io.FileInputStream; import java.io.FileInputStream;
import java.io.FileReader;
import java.io.IOException; import java.io.IOException;
import java.io.UnsupportedEncodingException; 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.storage.file.FileRepositoryBuilder;
import org.eclipse.jgit.test.resources.SampleDataRepositoryTestCase; import org.eclipse.jgit.test.resources.SampleDataRepositoryTestCase;
import org.eclipse.jgit.util.FileUtils; import org.eclipse.jgit.util.FileUtils;
import org.eclipse.jgit.util.IO;
import org.junit.Rule; import org.junit.Rule;
import org.junit.Test; import org.junit.Test;
import org.junit.rules.ExpectedException; 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", assertEquals("a many line\ncomment\n to test", c.getString("user",
null, "defaultCheckInComment")); null, "defaultCheckInComment"));
c.save(); c.save();
final FileReader fr = new FileReader(cfg);
final char[] cbuf = new char[configStr.length()]; // Saving normalizes out the weird "\\n\\\n" to a single escaped newline,
fr.read(cbuf); // and quotes the whole string.
fr.close(); final String expectedStr = " [core];comment\n\tfilemode = yes\n"
assertEquals(configStr, new String(cbuf)); + "[user]\n"
+ " email = A U Thor <thor@example.com> # 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 @Test

217
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 * Test reading of git config
*/ */
@SuppressWarnings("boxing")
public class ConfigTest { public class ConfigTest {
// A non-ASCII whitespace character: U+2002 EN QUAD.
private static final char WS = '\u2002';
@Rule @Rule
public ExpectedException expectedEx = ExpectedException.none(); public ExpectedException expectedEx = ExpectedException.none();
@ -666,28 +669,6 @@ public class ConfigTest {
assertTrue("Subsection should contain \"B\"", names.contains("B")); 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 @Test
public void testNoFinalNewline() throws ConfigInvalidException { public void testNoFinalNewline() throws ConfigInvalidException {
@ -956,53 +937,187 @@ public class ConfigTest {
@Test @Test
public void testEscapeSpacesOnly() throws ConfigInvalidException { 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(" "));
assertEquals("\" \"", Config.escapeValue(" "));
assertParseRoundTrip(" "); assertValueRoundTrip(" ", "\" \"");
assertParseRoundTrip(" "); assertValueRoundTrip(" ", "\" \"");
} }
@Test @Test
public void testEscapeLeadingSpace() throws ConfigInvalidException { public void testEscapeLeadingSpace() throws ConfigInvalidException {
assertEquals("x", Config.escapeValue("x")); assertValueRoundTrip("x", "x");
assertEquals("\" x\"", Config.escapeValue(" x")); assertValueRoundTrip(" x", "\" x\"");
assertEquals("\" x\"", Config.escapeValue(" x")); assertValueRoundTrip(" x", "\" x\"");
assertParseRoundTrip("x");
assertParseRoundTrip(" x");
assertParseRoundTrip(" x");
} }
@Test @Test
public void testEscapeTrailingSpace() throws ConfigInvalidException { public void testEscapeTrailingSpace() throws ConfigInvalidException {
assertEquals("x", Config.escapeValue("x")); assertValueRoundTrip("x", "x");
assertEquals("\"x \"", Config.escapeValue("x ")); assertValueRoundTrip("x ","\"x \"");
assertEquals("x\" \"", Config.escapeValue("x ")); assertValueRoundTrip("x ","\"x \"");
assertParseRoundTrip("x");
assertParseRoundTrip("x ");
assertParseRoundTrip("x ");
} }
@Test @Test
public void testEscapeLeadingAndTrailingSpace() public void testEscapeLeadingAndTrailingSpace()
throws ConfigInvalidException { throws ConfigInvalidException {
assertEquals("\" x \"", Config.escapeValue(" x ")); assertValueRoundTrip(" x ", "\" x \"");
assertEquals("\" x \"", Config.escapeValue(" x ")); assertValueRoundTrip(" x ", "\" x \"");
assertEquals("\" x \"", Config.escapeValue(" x ")); assertValueRoundTrip(" x ", "\" x \"");
assertEquals("\" x \"", Config.escapeValue(" 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\"");
}
assertParseRoundTrip(" x "); @Test
assertParseRoundTrip(" x "); public void testEscapeValueInvalidCharacters() {
assertParseRoundTrip(" x "); assertIllegalArgumentException(() -> Config.escapeSubsection("x\0y"));
assertParseRoundTrip(" x "); }
@Test
public void testEscapeSubsectionInvalidCharacters() {
assertIllegalArgumentException(() -> Config.escapeSubsection("x\ny"));
assertIllegalArgumentException(() -> Config.escapeSubsection("x\0y"));
} }
private static void assertParseRoundTrip(String value) @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 { throws ConfigInvalidException {
Config c = parse("[foo]\nbar = " + Config.escapeValue(value)); String escaped = Config.escapeValue(value);
assertEquals(value, c.getString("foo", null, "bar")); 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());
}
}
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 String parseEscapedSubsection(String escapedSubsection)
throws ConfigInvalidException {
String text = "[foo " + escapedSubsection + "]\nbar = value";
Config c = parse(text);
Set<String> 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());
}
} }
} }

3
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 commitOnRepoWithoutHEADCurrentlyNotSupported=Commit on repo without HEAD currently not supported
commitAmendOnInitialNotPossible=Amending is not possible on initial commit. commitAmendOnInitialNotPossible=Amending is not possible on initial commit.
compressingObjects=Compressing objects 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 connectionFailed=connection failed
connectionTimeOut=Connection time out: {0} connectionTimeOut=Connection time out: {0}
contextMustBeNonNegative=context must be >= 0 contextMustBeNonNegative=context must be >= 0

3
org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java

@ -191,6 +191,9 @@ public class JGitText extends TranslationBundle {
/***/ public String commitOnRepoWithoutHEADCurrentlyNotSupported; /***/ public String commitOnRepoWithoutHEADCurrentlyNotSupported;
/***/ public String commitAmendOnInitialNotPossible; /***/ public String commitAmendOnInitialNotPossible;
/***/ public String compressingObjects; /***/ public String compressingObjects;
/***/ public String configSubsectionContainsNewline;
/***/ public String configSubsectionContainsNullByte;
/***/ public String configValueContainsNullByte;
/***/ public String connectionFailed; /***/ public String connectionFailed;
/***/ public String connectionTimeOut; /***/ public String connectionTimeOut;
/***/ public String contextMustBeNonNegative; /***/ public String contextMustBeNonNegative;

187
org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java

@ -144,81 +144,91 @@ public class Config {
* the value to escape * the value to escape
* @return the escaped value * @return the escaped value
*/ */
static String escapeValue(final String x) { static String escapeValue(String x) {
if (x.isEmpty()) { if (x.isEmpty()) {
return ""; //$NON-NLS-1$ return ""; //$NON-NLS-1$
} }
boolean inquote = false;
int lineStart = 0; boolean needQuote = x.charAt(0) == ' ' || x.charAt(x.length() - 1) == ' ';
final StringBuilder r = new StringBuilder(x.length()); StringBuilder r = new StringBuilder(x.length());
for (int k = 0; k < x.length(); k++) { 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) { 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': case '\n':
if (inquote) { r.append('\\').append('n');
r.append('"');
inquote = false;
}
r.append("\\n\\\n"); //$NON-NLS-1$
lineStart = r.length();
break; break;
case '\t': case '\t':
r.append("\\t"); //$NON-NLS-1$ r.append('\\').append('t');
break; break;
case '\b': case '\b':
r.append("\\b"); //$NON-NLS-1$ r.append('\\').append('b');
break; break;
case '\\': case '\\':
r.append("\\\\"); //$NON-NLS-1$
break;
case '"': case '"':
r.append("\\\""); //$NON-NLS-1$ r.append('\\').append(c);
break; break;
case ';':
case '#': case '#':
if (!inquote) { case ';':
r.insert(lineStart, '"');
inquote = true;
}
r.append(c); r.append(c);
break; break;
case ' ':
if (!inquote && (r.length() == 0 || r.charAt(r.length() - 1) == ' ')) {
r.insert(lineStart, '"');
inquote = true;
}
r.append(' ');
break;
default: default:
thisCharNeedsQuote = false;
r.append(c); r.append(c);
break; break;
} }
needQuote |= thisCharNeedsQuote;
} }
if (!inquote) { return needQuote ? '"' + r.toString() + '"' : r.toString();
// 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;
} }
static String escapeSubsection(String x) {
if (x.isEmpty()) {
return "\"\""; //$NON-NLS-1$
} }
if (inquote) { StringBuilder r = new StringBuilder(x.length() + 2).append('"');
r.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); e.section = readSectionName(in);
input = in.read(); input = in.read();
if ('"' == input) { if ('"' == input) {
e.subsection = readValue(in, true, '"'); e.subsection = readSubsectionName(in);
input = in.read(); input = in.read();
} }
if (']' != input) if (']' != input)
@ -1085,7 +1095,7 @@ public class Config {
e.name = e.name.substring(0, e.name.length() - 1); e.name = e.name.substring(0, e.name.length() - 1);
e.value = MAGIC_EMPTY_VALUE; e.value = MAGIC_EMPTY_VALUE;
} else } else
e.value = readValue(in, false, -1); e.value = readValue(in);
if (e.section.equals("include")) { //$NON-NLS-1$ if (e.section.equals("include")) { //$NON-NLS-1$
addIncludedConfig(newEntries, e, depth); addIncludedConfig(newEntries, e, depth);
@ -1252,10 +1262,9 @@ public class Config {
return name.toString(); return name.toString();
} }
private static String readValue(final StringReader in, boolean quote, private static String readSubsectionName(StringReader in)
final int eol) throws ConfigInvalidException { throws ConfigInvalidException {
final StringBuilder value = new StringBuilder(); StringBuilder r = new StringBuilder();
boolean space = false;
for (;;) { for (;;) {
int c = in.read(); int c = in.read();
if (c < 0) { if (c < 0) {
@ -1263,30 +1272,80 @@ public class Config {
} }
if ('\n' == c) { if ('\n' == c) {
if (quote) throw new ConfigInvalidException(
throw new ConfigInvalidException(JGitText.get().newlineInQuotesNotAllowed); JGitText.get().newlineInQuotesNotAllowed);
in.reset();
break;
} }
if ('\\' == c) {
c = in.read();
switch (c) {
case -1:
throw new ConfigInvalidException(JGitText.get().endOfFileInEscape);
case '\\':
case '"':
r.append((char) c);
continue;
if (eol == c) default:
throw new ConfigInvalidException(MessageFormat.format(
JGitText.get().badEscape,
Character.valueOf(((char) c))));
}
}
if ('"' == c) {
break; break;
}
if (!quote) { r.append((char) c);
if (Character.isWhitespace((char) c)) { }
space = true; return r.toString();
continue; }
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);
} }
if (';' == c || '#' == c) {
in.reset(); in.reset();
break; break;
} }
if (!quote && (';' == c || '#' == c)) {
if (trailingSpaces != null) {
trailingSpaces.setLength(0);
}
in.reset();
break;
} }
if (space) { char cc = (char) c;
if (value.length() > 0) if (Character.isWhitespace(cc)) {
value.append(' '); if (inLeadingSpace) {
space = false; continue;
}
if (trailingSpaces == null) {
trailingSpaces = new StringBuilder();
}
trailingSpaces.append(cc);
continue;
} else {
inLeadingSpace = false;
if (trailingSpaces != null) {
value.append(trailingSpaces);
trailingSpaces.setLength(0);
}
} }
if ('\\' == c) { if ('\\' == c) {
@ -1323,7 +1382,7 @@ public class Config {
continue; continue;
} }
value.append((char) c); value.append(cc);
} }
return value.length() > 0 ? value.toString() : null; return value.length() > 0 ? value.toString() : null;
} }

Loading…
Cancel
Save