From 2c4841f18dd38e715ce2ed7ee078847fa570d7e3 Mon Sep 17 00:00:00 2001 From: Peter Edge Date: Mon, 28 Mar 2016 23:54:47 +0200 Subject: [PATCH 1/3] Do percent encoding matching grpc-java for the statusDesc. Signed-off-by: Peter Edge --- transport/handler_server.go | 2 +- transport/handler_server_test.go | 4 +-- transport/http2_server.go | 2 +- transport/http_util.go | 2 +- transport/transport.go | 48 ++++++++++++++++++++++++++++++++ transport/transport_test.go | 26 +++++++++++++++++ 6 files changed, 79 insertions(+), 5 deletions(-) diff --git a/transport/handler_server.go b/transport/handler_server.go index d7e18a0b6..490de9323 100644 --- a/transport/handler_server.go +++ b/transport/handler_server.go @@ -192,7 +192,7 @@ func (ht *serverHandlerTransport) WriteStatus(s *Stream, statusCode codes.Code, h := ht.rw.Header() h.Set("Grpc-Status", fmt.Sprintf("%d", statusCode)) if statusDesc != "" { - h.Set("Grpc-Message", statusDesc) + h.Set("Grpc-Message", grpcMessageEncode(statusDesc)) } if md := s.Trailer(); len(md) > 0 { for k, vv := range md { diff --git a/transport/handler_server_test.go b/transport/handler_server_test.go index 1fee72ff2..0711d012e 100644 --- a/transport/handler_server_test.go +++ b/transport/handler_server_test.go @@ -333,7 +333,7 @@ func handleStreamCloseBodyTest(t *testing.T, statusCode codes.Code, msg string) "Content-Type": {"application/grpc"}, "Trailer": {"Grpc-Status", "Grpc-Message"}, "Grpc-Status": {fmt.Sprint(uint32(statusCode))}, - "Grpc-Message": {msg}, + "Grpc-Message": {grpcMessageEncode(msg)}, } if !reflect.DeepEqual(st.rw.HeaderMap, wantHeader) { t.Errorf("Header+Trailer mismatch.\n got: %#v\nwant: %#v", st.rw.HeaderMap, wantHeader) @@ -381,7 +381,7 @@ func TestHandlerTransport_HandleStreams_Timeout(t *testing.T) { "Content-Type": {"application/grpc"}, "Trailer": {"Grpc-Status", "Grpc-Message"}, "Grpc-Status": {"4"}, - "Grpc-Message": {"too slow"}, + "Grpc-Message": {grpcMessageEncode("too slow")}, } if !reflect.DeepEqual(rw.HeaderMap, wantHeader) { t.Errorf("Header+Trailer Map mismatch.\n got: %#v\nwant: %#v", rw.HeaderMap, wantHeader) diff --git a/transport/http2_server.go b/transport/http2_server.go index 031642362..7ecd209c4 100644 --- a/transport/http2_server.go +++ b/transport/http2_server.go @@ -485,7 +485,7 @@ func (t *http2Server) WriteStatus(s *Stream, statusCode codes.Code, statusDesc s Name: "grpc-status", Value: strconv.Itoa(int(statusCode)), }) - t.hEnc.WriteField(hpack.HeaderField{Name: "grpc-message", Value: statusDesc}) + t.hEnc.WriteField(hpack.HeaderField{Name: "grpc-message", Value: grpcMessageEncode(statusDesc)}) // Attach the trailer metadata. for k, v := range s.trailer { for _, entry := range v { diff --git a/transport/http_util.go b/transport/http_util.go index 73c12d5f9..7e92f63d2 100644 --- a/transport/http_util.go +++ b/transport/http_util.go @@ -149,7 +149,7 @@ func (d *decodeState) processHeaderField(f hpack.HeaderField) { } d.statusCode = codes.Code(code) case "grpc-message": - d.statusDesc = f.Value + d.statusDesc = grpcMessageDecode(f.Value) case "grpc-timeout": d.timeoutSet = true var err error diff --git a/transport/transport.go b/transport/transport.go index 6eca1b3b4..1d44974ff 100644 --- a/transport/transport.go +++ b/transport/transport.go @@ -43,6 +43,7 @@ import ( "fmt" "io" "net" + "strconv" "sync" "time" @@ -506,3 +507,50 @@ func wait(ctx context.Context, closing <-chan struct{}, proceed <-chan int) (int return i, nil } } + +const ( + spaceByte = byte(int(' ')) + tildaByte = byte(int('~')) + percentByte = byte(int('%')) +) + +// matching https://github.com/grpc/grpc-java/pull/1517/files +func grpcMessageEncode(grpcMessage string) string { + if grpcMessage == "" { + return "" + } + var buffer bytes.Buffer + for _, c := range []byte(grpcMessage) { + if c >= spaceByte && c < tildaByte && c != percentByte { + _ = buffer.WriteByte(c) + } else { + _, _ = buffer.WriteString(fmt.Sprintf("%%%02X", c)) + } + } + return buffer.String() +} + +// matching https://github.com/grpc/grpc-java/pull/1517/files +func grpcMessageDecode(encodedGrpcMessage string) string { + if encodedGrpcMessage == "" { + return "" + } + var buffer bytes.Buffer + data := []byte(encodedGrpcMessage) + lenData := len(data) + for i := 0; i < lenData; i++ { + c := data[i] + if c == percentByte && i+2 < lenData { + parsed, err := strconv.ParseInt(string(data[i+1:i+3]), 16, 8) + if err != nil { + _ = buffer.WriteByte(c) + } else { + _ = buffer.WriteByte(byte(parsed)) + i += 2 + } + } else { + _ = buffer.WriteByte(c) + } + } + return buffer.String() +} diff --git a/transport/transport_test.go b/transport/transport_test.go index c9a95328a..6a15bf301 100644 --- a/transport/transport_test.go +++ b/transport/transport_test.go @@ -720,3 +720,29 @@ func TestIsReservedHeader(t *testing.T) { } } } + +func TestGrpcMessageEncode(t *testing.T) { + testGrpcMessageEncode(t, "my favorite character is \u0000", "my favorite character is %00") + testGrpcMessageEncode(t, "my favorite character is %", "my favorite character is %25") +} + +func TestGrpcMessageDecode(t *testing.T) { + testGrpcMessageDecode(t, "Hello", "Hello") + testGrpcMessageDecode(t, "H%61o", "Hao") + testGrpcMessageDecode(t, "H%6", "H%6") + testGrpcMessageDecode(t, "%G0", "%G0") +} + +func testGrpcMessageEncode(t *testing.T, input string, expected string) { + actual := grpcMessageEncode(input) + if expected != actual { + t.Errorf("Expected %s from grpcMessageEncode, got %s", expected, actual) + } +} + +func testGrpcMessageDecode(t *testing.T, input string, expected string) { + actual := grpcMessageDecode(input) + if expected != actual { + t.Errorf("Expected %s from grpcMessageDecode, got %s", expected, actual) + } +} From 4126905758b7695885656def00b638b2cc5b2f59 Mon Sep 17 00:00:00 2001 From: Peter Edge Date: Tue, 29 Mar 2016 01:13:11 +0200 Subject: [PATCH 2/3] Update grpcMessage encoding and decoding per review comments. Signed-off-by: Peter Edge --- transport/transport.go | 78 +++++++++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 27 deletions(-) diff --git a/transport/transport.go b/transport/transport.go index 1d44974ff..d4fb167e3 100644 --- a/transport/transport.go +++ b/transport/transport.go @@ -509,48 +509,72 @@ func wait(ctx context.Context, closing <-chan struct{}, proceed <-chan int) (int } const ( - spaceByte = byte(int(' ')) - tildaByte = byte(int('~')) - percentByte = byte(int('%')) + spaceByte = ' ' + tildaByte = '~' + percentByte = '%' ) -// matching https://github.com/grpc/grpc-java/pull/1517/files -func grpcMessageEncode(grpcMessage string) string { - if grpcMessage == "" { +// grpcMessageEncode encodes the grpc-message field in the same +// manner as https://github.com/grpc/grpc-java/pull/1517. +func grpcMessageEncode(msg string) string { + if msg == "" { return "" } - var buffer bytes.Buffer - for _, c := range []byte(grpcMessage) { - if c >= spaceByte && c < tildaByte && c != percentByte { - _ = buffer.WriteByte(c) - } else { - _, _ = buffer.WriteString(fmt.Sprintf("%%%02X", c)) + lenMsg := len(msg) + for i := 0; i < lenMsg; i++ { + c := msg[i] + if !(c >= spaceByte && c < tildaByte && c != percentByte) { + return grpcMessageEncodeUnchecked(msg) } } - return buffer.String() + return msg } -// matching https://github.com/grpc/grpc-java/pull/1517/files -func grpcMessageDecode(encodedGrpcMessage string) string { - if encodedGrpcMessage == "" { +func grpcMessageEncodeUnchecked(msg string) string { + var buf bytes.Buffer + lenMsg := len(msg) + for i := 0; i < lenMsg; i++ { + c := msg[i] + if c >= spaceByte && c < tildaByte && c != percentByte { + _ = buf.WriteByte(c) + } else { + _, _ = buf.WriteString(fmt.Sprintf("%%%02X", c)) + } + } + return buf.String() +} + +// grpcMessageDecode decodes the grpc-message field in the same +// manner as https://github.com/grpc/grpc-java/pull/1517. +func grpcMessageDecode(msg string) string { + if msg == "" { return "" } - var buffer bytes.Buffer - data := []byte(encodedGrpcMessage) - lenData := len(data) - for i := 0; i < lenData; i++ { - c := data[i] - if c == percentByte && i+2 < lenData { - parsed, err := strconv.ParseInt(string(data[i+1:i+3]), 16, 8) + lenMsg := len(msg) + for i := 0; i < lenMsg; i++ { + if msg[i] == percentByte && i+2 < lenMsg { + return grpcMessageDecodeUnchecked(msg) + } + } + return msg +} + +func grpcMessageDecodeUnchecked(msg string) string { + var buf bytes.Buffer + lenMsg := len(msg) + for i := 0; i < lenMsg; i++ { + c := msg[i] + if c == percentByte && i+2 < lenMsg { + parsed, err := strconv.ParseInt(msg[i+1:i+3], 16, 8) if err != nil { - _ = buffer.WriteByte(c) + _ = buf.WriteByte(c) } else { - _ = buffer.WriteByte(byte(parsed)) + _ = buf.WriteByte(byte(parsed)) i += 2 } } else { - _ = buffer.WriteByte(c) + _ = buf.WriteByte(c) } } - return buffer.String() + return buf.String() } From 9ae8316e29986b7973e8f2c61df36f8d3f1b95d7 Mon Sep 17 00:00:00 2001 From: Peter Edge Date: Tue, 29 Mar 2016 01:39:20 +0200 Subject: [PATCH 3/3] Change transport test for malformed status to encoding required status. Signed-off-by: Peter Edge --- transport/transport_test.go | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/transport/transport_test.go b/transport/transport_test.go index 6a15bf301..9c4147cb0 100644 --- a/transport/transport_test.go +++ b/transport/transport_test.go @@ -37,6 +37,7 @@ import ( "bytes" "fmt" "io" + "io/ioutil" "math" "net" "reflect" @@ -75,7 +76,7 @@ const ( normal hType = iota suspended misbehaved - malformedStatus + encodingRequiredStatus ) func (h *testStreamHandler) handleStream(t *testing.T, s *Stream) { @@ -128,9 +129,8 @@ func (h *testStreamHandler) handleStreamMisbehave(t *testing.T, s *Stream) { } } -func (h *testStreamHandler) handleStreamMalformedStatus(t *testing.T, s *Stream) { - // raw newline is not accepted by http2 framer and a http2.StreamError is - // generated. +func (h *testStreamHandler) handleStreamEncodingRequiredStatus(t *testing.T, s *Stream) { + // raw newline is not accepted by http2 framer so it must be encoded. h.t.WriteStatus(s, codes.Internal, "\n") } @@ -179,9 +179,9 @@ func (s *server) start(t *testing.T, port int, maxStreams uint32, ht hType) { go transport.HandleStreams(func(s *Stream) { go h.handleStreamMisbehave(t, s) }) - case malformedStatus: + case encodingRequiredStatus: go transport.HandleStreams(func(s *Stream) { - go h.handleStreamMalformedStatus(t, s) + go h.handleStreamEncodingRequiredStatus(t, s) }) default: go transport.HandleStreams(func(s *Stream) { @@ -663,8 +663,8 @@ func TestClientWithMisbehavedServer(t *testing.T) { server.stop() } -func TestMalformedStatus(t *testing.T) { - server, ct := setUp(t, 0, math.MaxUint32, malformedStatus) +func TestEncodingRequiredStatus(t *testing.T) { + server, ct := setUp(t, 0, math.MaxUint32, encodingRequiredStatus) callHdr := &CallHdr{ Host: "localhost", Method: "foo", @@ -680,10 +680,8 @@ func TestMalformedStatus(t *testing.T) { if err := ct.Write(s, expectedRequest, &opts); err != nil { t.Fatalf("Failed to write the request: %v", err) } - p := make([]byte, http2MaxFrameLen) - expectedErr := StreamErrorf(codes.Internal, "invalid header field value \"\\n\"") - if _, err = s.dec.Read(p); err != expectedErr { - t.Fatalf("Read the err %v, want %v", err, expectedErr) + if _, err = ioutil.ReadAll(s); err != nil { + t.Fatal(err) } ct.Close() server.stop()