[tor-commits] [goptlib/master] Don't escape, just panic in formatline.

[dcf at torproject.org]

commit 5791268c391c9388d664c654e2278a50463c2c74
Author: David Fifield <david at bamsoftware.com>
Date:   Wed Oct 8 15:39:45 2014 -0700

    Don't escape, just panic in formatline.
    
    Backslash escaping in formatline was conflicting with pt-spec.txt's
    specified backslash escaping for SMETHOD ARGS.
    
    https://trac.torproject.org/projects/tor/ticket/13370
    
    Arguably we could just ignore any illegal bytes, but I made it panic in
    order to make any application bugs more obvious.
---
 pt.go      |   56 +++++++++++++++++++++++++++++--------------
 pt_test.go |   77 +++++++++++++++++++++++++++++++++++++-----------------------
 2 files changed, 86 insertions(+), 47 deletions(-)

diff --git a/pt.go b/pt.go
index 62dfc38..b02b9c2 100644
--- a/pt.go
+++ b/pt.go
@@ -201,35 +201,57 @@ func getenvRequired(key string) (string, error) {
 	return value, nil
 }
 
-// Escape a string so it contains no byte values over 127 and doesn't contain
-// any of the characters '\x00' or '\n'.
-func escape(s string) string {
-	var buf bytes.Buffer
-	for _, b := range []byte(s) {
-		if b == '\n' {
-			buf.WriteString("\\n")
-		} else if b == '\\' {
-			buf.WriteString("\\\\")
-		} else if 0 < b && b < 128 {
-			buf.WriteByte(b)
-		} else {
-			fmt.Fprintf(&buf, "\\x%02x", b)
+// Returns true iff keyword contains only bytes allowed in a PT→Tor output line
+// keyword.
+// <KeywordChar> ::= <any US-ASCII alphanumeric, dash, and underscore>
+func keywordIsSafe(keyword string) bool {
+	for _, b := range []byte(keyword) {
+		if b >= '0' && b <= '9' {
+			continue
+		}
+		if b >= 'A' && b <= 'Z' {
+			continue
+		}
+		if b >= 'a' && b <= 'z' {
+			continue
 		}
+		if b == '-' || b == '_' {
+			continue
+		}
+		return false
 	}
-	return buf.String()
+	return true
+}
+
+// Returns true iff arg contains only bytes allowed in a PT→Tor output line arg.
+// <ArgChar> ::= <any US-ASCII character but NUL or NL>
+func argIsSafe(arg string) bool {
+	for _, b := range []byte(arg) {
+		if b >= '\x80' || b == '\x00' || b == '\n' {
+			return false
+		}
+	}
+	return true
 }
 
 func formatline(keyword string, v ...string) string {
 	var buf bytes.Buffer
+	if !keywordIsSafe(keyword) {
+		panic(fmt.Sprintf("keyword %q contains forbidden bytes", keyword))
+	}
 	buf.WriteString(keyword)
 	for _, x := range v {
-		buf.WriteString(" " + escape(x))
+		if !argIsSafe(x) {
+			panic(fmt.Sprintf("arg %q contains forbidden bytes", x))
+		}
+		buf.WriteString(" " + x)
 	}
 	return buf.String()
 }
 
-// Print a pluggable transports protocol line to Stdout. The line consists of an
-// unescaped keyword, followed by any number of escaped strings.
+// Print a pluggable transports protocol line to Stdout. The line consists of a
+// keyword followed by any number of space-separated arg strings. Panics if
+// there are forbidden bytes in the keyword or the args (pt-spec.txt 2.2.1).
 func line(keyword string, v ...string) {
 	fmt.Fprintln(Stdout, formatline(keyword, v...))
 }
diff --git a/pt_test.go b/pt_test.go
index 456313a..983bf55 100644
--- a/pt_test.go
+++ b/pt_test.go
@@ -13,43 +13,60 @@ import (
 	"testing"
 )
 
-func stringIsSafe(s string) bool {
-	for _, c := range []byte(s) {
-		if c == '\x00' || c == '\n' || c > 127 {
-			return false
+func TestKeywordIsSafe(t *testing.T) {
+	tests := [...]struct {
+		keyword  string
+		expected bool
+	}{
+		{"", true},
+		{"0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ-_", true},
+		{"CMETHOD", true},
+		{"CMETHOD:", false},
+		{"a b c", false},
+		{"CMETHOD\x7f", false},
+		{"CMETHOD\x80", false},
+		{"CMETHOD\x81", false},
+		{"CMETHOD\xff", false},
+		{"CMÉTHOD", false},
+	}
+
+	for _, input := range tests {
+		isSafe := keywordIsSafe(input.keyword)
+		if isSafe != input.expected {
+			t.Errorf("keywordIsSafe(%q) → %v (expected %v)",
+				input.keyword, isSafe, input.expected)
 		}
 	}
-	return true
 }
 
-func TestEscape(t *testing.T) {
-	tests := [...]string{
-		"",
-		"abc",
-		"a\nb",
-		"a\\b",
-		"ab\\",
-		"ab\\\n",
-		"ab\n\\",
+func TestArgIsSafe(t *testing.T) {
+	tests := [...]struct {
+		arg      string
+		expected bool
+	}{
+		{"", true},
+		{"abc", true},
+		{"127.0.0.1:8000", true},
+		{"étude", false},
+		{"a\nb", false},
+		{"a\\b", true},
+		{"ab\\", true},
+		{"ab\\\n", false},
+		{"ab\n\\", false},
+		{"abc\x7f", true},
+		{"abc\x80", false},
+		{"abc\x81", false},
+		{"abc\xff", false},
+		{"abc\xff", false},
+		{"var=GVsbG8\\=", true},
 	}
 
-	check := func(input string) {
-		output := escape(input)
-		if stringIsSafe(input) && input != output {
-			t.Errorf("escape(%q) → %q despite being safe", input, output)
-		}
-		if !stringIsSafe(output) {
-			t.Errorf("escape(%q) → %q is not safe", input, output)
-		}
-	}
 	for _, input := range tests {
-		check(input)
-	}
-	for b := 0; b < 256; b++ {
-		// check one-byte string with each byte value 0–255
-		check(string([]byte{byte(b)}))
-		// check UTF-8 encoding of each character 0–255
-		check(string(b))
+		isSafe := argIsSafe(input.arg)
+		if isSafe != input.expected {
+			t.Errorf("argIsSafe(%q) → %v (expected %v)",
+				input.arg, isSafe, input.expected)
+		}
 	}
 }