From 6399d6ea1c3b0396997d32fb8d1ab665d7c933ff Mon Sep 17 00:00:00 2001 From: Patrick Date: Tue, 19 Oct 2021 16:56:42 +0800 Subject: [PATCH 01/26] add metadata validate function --- metadata/metadata.go | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/metadata/metadata.go b/metadata/metadata.go index 3604c7819fd..f6807df98eb 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -23,6 +23,7 @@ package metadata // import "google.golang.org/grpc/metadata" import ( "context" + "errors" "fmt" "strings" ) @@ -82,6 +83,42 @@ func Pairs(kv ...string) MD { return md } +// Validate returns error if md is not valid +// There are check items: +// - header names contain one or more characters from this set [0-9 a-z _ - .] +// - if the header-name ends with a "-bin" suffix, the header-value could contain an arbitrary octet sequence. So no real validation required here. +// - if header-name does not end with a "-bin" suffix, header-value should only contain one or more characters from the set ( %x20-%x7E ) which includes space and printable ASCII. +func (md MD) Validate() error { + for k, vals := range md { + // check key + for _, rc := range []rune(k) { + if !(rc >= 'a' && rc <= 'z') && !(rc >= '0' && rc <= '9') && rc != '.' || rc != '-' || rc != '_' { + return errors.New("header key is not 0-9a-z-_.") + } + } + if strings.HasSuffix(k, "-bin") { + continue + } + // check value + for _, val := range vals { + if hasNotPrintable(val) { + return errors.New("header val has not printable ASCII") + } + } + } + return nil +} + +// hasNotPrintable return true if msg has character not in %x20-%x7E +func hasNotPrintable(msg string) bool { + for _, rc := range []rune(msg) { + if rc < 0x20 || rc > 0x7E { + return true + } + } + return false +} + // Len returns the number of items in md. func (md MD) Len() int { return len(md) From aec9260297dc8885e2789d9f96e39b1860de7caa Mon Sep 17 00:00:00 2001 From: Patrick Date: Tue, 19 Oct 2021 20:18:58 +0800 Subject: [PATCH 02/26] add metadata validate unit test and fix key checker --- metadata/metadata.go | 2 +- metadata/metadata_test.go | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/metadata/metadata.go b/metadata/metadata.go index f6807df98eb..227c732df05 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -92,7 +92,7 @@ func (md MD) Validate() error { for k, vals := range md { // check key for _, rc := range []rune(k) { - if !(rc >= 'a' && rc <= 'z') && !(rc >= '0' && rc <= '9') && rc != '.' || rc != '-' || rc != '_' { + if !(rc >= 'a' && rc <= 'z') && !(rc >= '0' && rc <= '9') && rc != '.' && rc != '-' && rc != '_' { return errors.New("header key is not 0-9a-z-_.") } } diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index 89be06eaada..d6dc09c0239 100644 --- a/metadata/metadata_test.go +++ b/metadata/metadata_test.go @@ -262,6 +262,29 @@ func (s) TestAppendToOutgoingContext_FromKVSlice(t *testing.T) { } } +func (s) TestValidate(t *testing.T) { + // check key + md := MD{} + md.Set(string(rune(0x19)), "testVal") + err := md.Validate() + if err == nil { + t.Errorf("validating md, err should not nil") + } + // check value + md = MD{} + md.Set("test", string(rune(0x19))) + err = md.Validate() + if err == nil { + t.Errorf("validating md, err should not nil") + } + md = MD{} + md.Set("test-bin", string(rune(0x19))) + err = md.Validate() + if err != nil { + t.Fatalf("when key suffix '-bin' and value is binary, validate wrong msg{%v}", err) + } +} + // Old/slow approach to adding metadata to context func Benchmark_AddingMetadata_ContextManipulationApproach(b *testing.B) { // TODO: Add in N=1-100 tests once Go1.6 support is removed. From b6f263e9a899fb5b1e59c3659180029c3a30e238 Mon Sep 17 00:00:00 2001 From: Patrick Date: Tue, 23 Nov 2021 10:22:09 +0800 Subject: [PATCH 03/26] add validate add server stream and grpc.Invoke --- call.go | 7 +++++++ stream.go | 14 +++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/call.go b/call.go index 9e20e4d385f..b9147ecb121 100644 --- a/call.go +++ b/call.go @@ -20,6 +20,8 @@ package grpc import ( "context" + + "google.golang.org/grpc/metadata" ) // Invoke sends the RPC request on the wire and returns after response is @@ -27,6 +29,11 @@ import ( // // All errors returned by Invoke are compatible with the status package. func (cc *ClientConn) Invoke(ctx context.Context, method string, args, reply interface{}, opts ...CallOption) error { + if md, _ ,ok := metadata.FromOutgoingContextRaw(ctx); ok { + if err := md.Validate(); err != nil { + return err + } + } // allow interceptor to see all applicable call options, which means those // configured as defaults from dial option as well as per-call options opts = combine(cc.dopts.callOptions, opts) diff --git a/stream.go b/stream.go index 625d47b34e5..f5a6cea69c6 100644 --- a/stream.go +++ b/stream.go @@ -1446,11 +1446,20 @@ func (ss *serverStream) SetHeader(md metadata.MD) error { if md.Len() == 0 { return nil } + err := md.Validate() + if err != nil { + return err + } return ss.s.SetHeader(md) } func (ss *serverStream) SendHeader(md metadata.MD) error { - err := ss.t.WriteHeader(ss.s, md) + err := md.Validate() + if err != nil { + return err + } + + err = ss.t.WriteHeader(ss.s, md) if ss.binlog != nil && !ss.serverHeaderBinlogged { h, _ := ss.s.Header() ss.binlog.Log(&binarylog.ServerHeader{ @@ -1465,6 +1474,9 @@ func (ss *serverStream) SetTrailer(md metadata.MD) { if md.Len() == 0 { return } + if md.Validate() != nil { + return + } ss.s.SetTrailer(md) } From 2520260e863ad08b48e1c872872a8aad6c2bc2dd Mon Sep 17 00:00:00 2001 From: Patrick Date: Wed, 1 Dec 2021 15:05:55 +0800 Subject: [PATCH 04/26] make TestValidate a table driven test --- metadata/metadata_test.go | 41 +++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index d6dc09c0239..6a96f34c573 100644 --- a/metadata/metadata_test.go +++ b/metadata/metadata_test.go @@ -20,6 +20,7 @@ package metadata import ( "context" + "errors" "reflect" "strconv" "testing" @@ -263,25 +264,27 @@ func (s) TestAppendToOutgoingContext_FromKVSlice(t *testing.T) { } func (s) TestValidate(t *testing.T) { - // check key - md := MD{} - md.Set(string(rune(0x19)), "testVal") - err := md.Validate() - if err == nil { - t.Errorf("validating md, err should not nil") - } - // check value - md = MD{} - md.Set("test", string(rune(0x19))) - err = md.Validate() - if err == nil { - t.Errorf("validating md, err should not nil") - } - md = MD{} - md.Set("test-bin", string(rune(0x19))) - err = md.Validate() - if err != nil { - t.Fatalf("when key suffix '-bin' and value is binary, validate wrong msg{%v}", err) + for _, test := range []struct { + md MD + want error + }{ + { + md: Pairs(string(rune(0x19)), "testVal"), + want: errors.New("header key is not 0-9a-z-_."), + }, + { + md: Pairs("test", string(rune(0x19))), + want: errors.New("header val has not printable ASCII"), + }, + { + md: Pairs("test-bin", string(rune(0x19))), + want: nil, + }, + } { + err := test.md.Validate() + if !reflect.DeepEqual(err, test.want) { + t.Errorf("validating metadata which is %v got err :%v, want err :%v", test.md, err, test.want) + } } } From 8625b252e39866971c3376fb7a2b8201299469cf Mon Sep 17 00:00:00 2001 From: Patrick Date: Tue, 7 Dec 2021 17:46:40 +0800 Subject: [PATCH 05/26] optimize for --- metadata/metadata.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/metadata/metadata.go b/metadata/metadata.go index 227c732df05..6e514d48229 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -90,9 +90,10 @@ func Pairs(kv ...string) MD { // - if header-name does not end with a "-bin" suffix, header-value should only contain one or more characters from the set ( %x20-%x7E ) which includes space and printable ASCII. func (md MD) Validate() error { for k, vals := range md { - // check key - for _, rc := range []rune(k) { - if !(rc >= 'a' && rc <= 'z') && !(rc >= '0' && rc <= '9') && rc != '.' && rc != '-' && rc != '_' { + // check key, for i that saving a conversion if not using for range + for i := 0; i< len(k); i++ { + r := k[i] + if !(r >= 'a' && r <= 'z') && !(r >= '0' && r <= '9') && r != '.' && r != '-' && r != '_' { return errors.New("header key is not 0-9a-z-_.") } } @@ -111,8 +112,9 @@ func (md MD) Validate() error { // hasNotPrintable return true if msg has character not in %x20-%x7E func hasNotPrintable(msg string) bool { - for _, rc := range []rune(msg) { - if rc < 0x20 || rc > 0x7E { + // for i that saving a conversion if not using for range + for i := 0; i < len(msg); i++ { + if msg[i] < 0x20 || msg[i] > 0x7E { return true } } From b848e9bb9d1e6e4d06bc1d3fafec3ad21b478830 Mon Sep 17 00:00:00 2001 From: Patrick Date: Wed, 8 Dec 2021 10:40:36 +0800 Subject: [PATCH 06/26] move Validate to internal.metadata --- call.go | 3 +- internal/metadata/metadata.go | 41 +++++++++++++++++++++++ internal/metadata/metadata_test.go | 54 ++++++++++++++++++++++++++++++ metadata/metadata.go | 39 --------------------- metadata/metadata_test.go | 25 -------------- stream.go | 7 ++-- 6 files changed, 101 insertions(+), 68 deletions(-) diff --git a/call.go b/call.go index b9147ecb121..9b3fb7004ea 100644 --- a/call.go +++ b/call.go @@ -22,6 +22,7 @@ import ( "context" "google.golang.org/grpc/metadata" + imetadata "google.golang.org/grpc/internal/metadata" ) // Invoke sends the RPC request on the wire and returns after response is @@ -30,7 +31,7 @@ import ( // All errors returned by Invoke are compatible with the status package. func (cc *ClientConn) Invoke(ctx context.Context, method string, args, reply interface{}, opts ...CallOption) error { if md, _ ,ok := metadata.FromOutgoingContextRaw(ctx); ok { - if err := md.Validate(); err != nil { + if err := imetadata.Validate(md); err != nil { return err } } diff --git a/internal/metadata/metadata.go b/internal/metadata/metadata.go index b8733dbf340..a5ad6878c0d 100644 --- a/internal/metadata/metadata.go +++ b/internal/metadata/metadata.go @@ -22,6 +22,9 @@ package metadata import ( + "errors" + "strings" + "google.golang.org/grpc/metadata" "google.golang.org/grpc/resolver" ) @@ -72,3 +75,41 @@ func Set(addr resolver.Address, md metadata.MD) resolver.Address { addr.Attributes = addr.Attributes.WithValue(mdKey, mdValue(md)) return addr } + +// Validate returns error if md is not valid +// There are check items: +// - header names contain one or more characters from this set [0-9 a-z _ - .] +// - if the header-name ends with a "-bin" suffix, the header-value could contain an arbitrary octet sequence. So no real validation required here. +// - if header-name does not end with a "-bin" suffix, header-value should only contain one or more characters from the set ( %x20-%x7E ) which includes space and printable ASCII. +func Validate(md metadata.MD) error { + for k, vals := range md { + // check key, for i that saving a conversion if not using for range + for i := 0; i< len(k); i++ { + r := k[i] + if !(r >= 'a' && r <= 'z') && !(r >= '0' && r <= '9') && r != '.' && r != '-' && r != '_' { + return errors.New("header key is not 0-9a-z-_.") + } + } + if strings.HasSuffix(k, "-bin") { + continue + } + // check value + for _, val := range vals { + if hasNotPrintable(val) { + return errors.New("header val has not printable ASCII") + } + } + } + return nil +} + +// hasNotPrintable return true if msg has character not in %x20-%x7E +func hasNotPrintable(msg string) bool { + // for i that saving a conversion if not using for range + for i := 0; i < len(msg); i++ { + if msg[i] < 0x20 || msg[i] > 0x7E { + return true + } + } + return false +} diff --git a/internal/metadata/metadata_test.go b/internal/metadata/metadata_test.go index 1aa0f9798e8..780cb82673a 100644 --- a/internal/metadata/metadata_test.go +++ b/internal/metadata/metadata_test.go @@ -19,6 +19,10 @@ package metadata import ( + "errors" + "fmt" + "reflect" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -84,3 +88,53 @@ func TestSet(t *testing.T) { }) } } + +func TestValidate(t *testing.T) { + for _, test := range []struct { + md metadata.MD + want error + }{ + { + md: Pairs(string(rune(0x19)), "testVal"), + want: errors.New("header key is not 0-9a-z-_."), + }, + { + md: Pairs("test", string(rune(0x19))), + want: errors.New("header val has not printable ASCII"), + }, + { + md: Pairs("test-bin", string(rune(0x19))), + want: nil, + }, + } { + err := Validate(test.md) + if !reflect.DeepEqual(err, test.want) { + t.Errorf("validating metadata which is %v got err :%v, want err :%v", test.md, err, test.want) + } + } +} + + +// Pairs returns an MD formed by the mapping of key, value ... +// Pairs panics if len(kv) is odd. +// +// Only the following ASCII characters are allowed in keys: +// - digits: 0-9 +// - uppercase letters: A-Z (normalized to lower) +// - lowercase letters: a-z +// - special characters: -_. +// Uppercase letters are automatically converted to lowercase. +// +// Keys beginning with "grpc-" are reserved for grpc-internal use only and may +// result in errors if set in metadata. +func Pairs(kv ...string) metadata.MD { + if len(kv)%2 == 1 { + panic(fmt.Sprintf("metadata: Pairs got the odd number of input pairs for metadata: %d", len(kv))) + } + md := metadata.MD{} + for i := 0; i < len(kv); i += 2 { + key := strings.ToLower(kv[i]) + md[key] = append(md[key], kv[i+1]) + } + return md +} diff --git a/metadata/metadata.go b/metadata/metadata.go index 6e514d48229..3604c7819fd 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -23,7 +23,6 @@ package metadata // import "google.golang.org/grpc/metadata" import ( "context" - "errors" "fmt" "strings" ) @@ -83,44 +82,6 @@ func Pairs(kv ...string) MD { return md } -// Validate returns error if md is not valid -// There are check items: -// - header names contain one or more characters from this set [0-9 a-z _ - .] -// - if the header-name ends with a "-bin" suffix, the header-value could contain an arbitrary octet sequence. So no real validation required here. -// - if header-name does not end with a "-bin" suffix, header-value should only contain one or more characters from the set ( %x20-%x7E ) which includes space and printable ASCII. -func (md MD) Validate() error { - for k, vals := range md { - // check key, for i that saving a conversion if not using for range - for i := 0; i< len(k); i++ { - r := k[i] - if !(r >= 'a' && r <= 'z') && !(r >= '0' && r <= '9') && r != '.' && r != '-' && r != '_' { - return errors.New("header key is not 0-9a-z-_.") - } - } - if strings.HasSuffix(k, "-bin") { - continue - } - // check value - for _, val := range vals { - if hasNotPrintable(val) { - return errors.New("header val has not printable ASCII") - } - } - } - return nil -} - -// hasNotPrintable return true if msg has character not in %x20-%x7E -func hasNotPrintable(msg string) bool { - // for i that saving a conversion if not using for range - for i := 0; i < len(msg); i++ { - if msg[i] < 0x20 || msg[i] > 0x7E { - return true - } - } - return false -} - // Len returns the number of items in md. func (md MD) Len() int { return len(md) diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index 6a96f34c573..8f14192209e 100644 --- a/metadata/metadata_test.go +++ b/metadata/metadata_test.go @@ -263,31 +263,6 @@ func (s) TestAppendToOutgoingContext_FromKVSlice(t *testing.T) { } } -func (s) TestValidate(t *testing.T) { - for _, test := range []struct { - md MD - want error - }{ - { - md: Pairs(string(rune(0x19)), "testVal"), - want: errors.New("header key is not 0-9a-z-_."), - }, - { - md: Pairs("test", string(rune(0x19))), - want: errors.New("header val has not printable ASCII"), - }, - { - md: Pairs("test-bin", string(rune(0x19))), - want: nil, - }, - } { - err := test.md.Validate() - if !reflect.DeepEqual(err, test.want) { - t.Errorf("validating metadata which is %v got err :%v, want err :%v", test.md, err, test.want) - } - } -} - // Old/slow approach to adding metadata to context func Benchmark_AddingMetadata_ContextManipulationApproach(b *testing.B) { // TODO: Add in N=1-100 tests once Go1.6 support is removed. diff --git a/stream.go b/stream.go index f5a6cea69c6..87519886cb5 100644 --- a/stream.go +++ b/stream.go @@ -36,6 +36,7 @@ import ( "google.golang.org/grpc/internal/channelz" "google.golang.org/grpc/internal/grpcrand" "google.golang.org/grpc/internal/grpcutil" + imetadata "google.golang.org/grpc/internal/metadata" iresolver "google.golang.org/grpc/internal/resolver" "google.golang.org/grpc/internal/serviceconfig" "google.golang.org/grpc/internal/transport" @@ -1446,7 +1447,7 @@ func (ss *serverStream) SetHeader(md metadata.MD) error { if md.Len() == 0 { return nil } - err := md.Validate() + err := imetadata.Validate(md) if err != nil { return err } @@ -1454,7 +1455,7 @@ func (ss *serverStream) SetHeader(md metadata.MD) error { } func (ss *serverStream) SendHeader(md metadata.MD) error { - err := md.Validate() + err := imetadata.Validate(md) if err != nil { return err } @@ -1474,7 +1475,7 @@ func (ss *serverStream) SetTrailer(md metadata.MD) { if md.Len() == 0 { return } - if md.Validate() != nil { + if imetadata.Validate(md) != nil { return } ss.s.SetTrailer(md) From 99522c3ea0196f21db788b00a61bf40c764a56d3 Mon Sep 17 00:00:00 2001 From: Patrick Date: Thu, 9 Dec 2021 20:47:24 +0800 Subject: [PATCH 07/26] update log and unittest --- internal/metadata/metadata.go | 2 +- internal/metadata/metadata_test.go | 31 +++--------------------------- metadata/metadata_test.go | 1 - 3 files changed, 4 insertions(+), 30 deletions(-) diff --git a/internal/metadata/metadata.go b/internal/metadata/metadata.go index a5ad6878c0d..55067bbe4d2 100644 --- a/internal/metadata/metadata.go +++ b/internal/metadata/metadata.go @@ -76,7 +76,7 @@ func Set(addr resolver.Address, md metadata.MD) resolver.Address { return addr } -// Validate returns error if md is not valid +// Validate returns an error if the input md contains invalid keys or values. // There are check items: // - header names contain one or more characters from this set [0-9 a-z _ - .] // - if the header-name ends with a "-bin" suffix, the header-value could contain an arbitrary octet sequence. So no real validation required here. diff --git a/internal/metadata/metadata_test.go b/internal/metadata/metadata_test.go index 780cb82673a..ce43c520ecb 100644 --- a/internal/metadata/metadata_test.go +++ b/internal/metadata/metadata_test.go @@ -95,15 +95,15 @@ func TestValidate(t *testing.T) { want error }{ { - md: Pairs(string(rune(0x19)), "testVal"), + md: metadata.Pairs(string(rune(0x19)), "testVal"), want: errors.New("header key is not 0-9a-z-_."), }, { - md: Pairs("test", string(rune(0x19))), + md: metadata.Pairs("test", string(rune(0x19))), want: errors.New("header val has not printable ASCII"), }, { - md: Pairs("test-bin", string(rune(0x19))), + md: metadata.Pairs("test-bin", string(rune(0x19))), want: nil, }, } { @@ -113,28 +113,3 @@ func TestValidate(t *testing.T) { } } } - - -// Pairs returns an MD formed by the mapping of key, value ... -// Pairs panics if len(kv) is odd. -// -// Only the following ASCII characters are allowed in keys: -// - digits: 0-9 -// - uppercase letters: A-Z (normalized to lower) -// - lowercase letters: a-z -// - special characters: -_. -// Uppercase letters are automatically converted to lowercase. -// -// Keys beginning with "grpc-" are reserved for grpc-internal use only and may -// result in errors if set in metadata. -func Pairs(kv ...string) metadata.MD { - if len(kv)%2 == 1 { - panic(fmt.Sprintf("metadata: Pairs got the odd number of input pairs for metadata: %d", len(kv))) - } - md := metadata.MD{} - for i := 0; i < len(kv); i += 2 { - key := strings.ToLower(kv[i]) - md[key] = append(md[key], kv[i+1]) - } - return md -} diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index 8f14192209e..89be06eaada 100644 --- a/metadata/metadata_test.go +++ b/metadata/metadata_test.go @@ -20,7 +20,6 @@ package metadata import ( "context" - "errors" "reflect" "strconv" "testing" From 4b8857188ae0a68d4c2870db3b9f1e43295cfb66 Mon Sep 17 00:00:00 2001 From: Patrick Date: Thu, 9 Dec 2021 21:17:36 +0800 Subject: [PATCH 08/26] not use pairs --- internal/metadata/metadata_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/metadata/metadata_test.go b/internal/metadata/metadata_test.go index ce43c520ecb..2d896d416ba 100644 --- a/internal/metadata/metadata_test.go +++ b/internal/metadata/metadata_test.go @@ -95,15 +95,15 @@ func TestValidate(t *testing.T) { want error }{ { - md: metadata.Pairs(string(rune(0x19)), "testVal"), + md: map[string][]string{string(rune(0x19)): []string{"testVal"}}, want: errors.New("header key is not 0-9a-z-_."), }, { - md: metadata.Pairs("test", string(rune(0x19))), + md: map[string][]string{"test": []string{string(rune(0x19))}, want: errors.New("header val has not printable ASCII"), }, { - md: metadata.Pairs("test-bin", string(rune(0x19))), + md: map[string][]string{"test-bin": []string{rune(0x19)}}, want: nil, }, } { From 0ac1e02ce9f2adff78efb9c30ec0c773ec3de303 Mon Sep 17 00:00:00 2001 From: Patrick Date: Fri, 10 Dec 2021 11:00:35 +0800 Subject: [PATCH 09/26] move validate to newClientStream and update comment --- call.go | 8 -------- internal/metadata/metadata.go | 3 ++- stream.go | 5 +++++ 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/call.go b/call.go index 9b3fb7004ea..9e20e4d385f 100644 --- a/call.go +++ b/call.go @@ -20,9 +20,6 @@ package grpc import ( "context" - - "google.golang.org/grpc/metadata" - imetadata "google.golang.org/grpc/internal/metadata" ) // Invoke sends the RPC request on the wire and returns after response is @@ -30,11 +27,6 @@ import ( // // All errors returned by Invoke are compatible with the status package. func (cc *ClientConn) Invoke(ctx context.Context, method string, args, reply interface{}, opts ...CallOption) error { - if md, _ ,ok := metadata.FromOutgoingContextRaw(ctx); ok { - if err := imetadata.Validate(md); err != nil { - return err - } - } // allow interceptor to see all applicable call options, which means those // configured as defaults from dial option as well as per-call options opts = combine(cc.dopts.callOptions, opts) diff --git a/internal/metadata/metadata.go b/internal/metadata/metadata.go index 55067bbe4d2..02c0dea7331 100644 --- a/internal/metadata/metadata.go +++ b/internal/metadata/metadata.go @@ -77,6 +77,7 @@ func Set(addr resolver.Address, md metadata.MD) resolver.Address { } // Validate returns an error if the input md contains invalid keys or values. +// // There are check items: // - header names contain one or more characters from this set [0-9 a-z _ - .] // - if the header-name ends with a "-bin" suffix, the header-value could contain an arbitrary octet sequence. So no real validation required here. @@ -103,7 +104,7 @@ func Validate(md metadata.MD) error { return nil } -// hasNotPrintable return true if msg has character not in %x20-%x7E +// hasNotPrintable return true if msg contains any characters which are not in %x20-%x7E func hasNotPrintable(msg string) bool { // for i that saving a conversion if not using for range for i := 0; i < len(msg); i++ { diff --git a/stream.go b/stream.go index 87519886cb5..f2f32be7396 100644 --- a/stream.go +++ b/stream.go @@ -165,6 +165,11 @@ func NewClientStream(ctx context.Context, desc *StreamDesc, cc *ClientConn, meth } func newClientStream(ctx context.Context, desc *StreamDesc, cc *ClientConn, method string, opts ...CallOption) (_ ClientStream, err error) { + if md, _ ,ok := metadata.FromOutgoingContextRaw(ctx); ok { + if err := imetadata.Validate(md); err != nil { + return nil, status.Error(codes.Internal, err.Error()) + } + } if channelz.IsOn() { cc.incrCallsStarted() defer func() { From 88ea32f62492d64e3c2cf25d0710d1a6ad9cc7e3 Mon Sep 17 00:00:00 2001 From: Patrick Date: Mon, 13 Dec 2021 14:16:16 +0800 Subject: [PATCH 10/26] fix unit test --- internal/metadata/metadata_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/internal/metadata/metadata_test.go b/internal/metadata/metadata_test.go index 2d896d416ba..eef4633c4ee 100644 --- a/internal/metadata/metadata_test.go +++ b/internal/metadata/metadata_test.go @@ -20,9 +20,7 @@ package metadata import ( "errors" - "fmt" "reflect" - "strings" "testing" "github.com/google/go-cmp/cmp" @@ -95,15 +93,15 @@ func TestValidate(t *testing.T) { want error }{ { - md: map[string][]string{string(rune(0x19)): []string{"testVal"}}, + md: map[string][]string{string(rune(0x19)): {"testVal"}}, want: errors.New("header key is not 0-9a-z-_."), }, { - md: map[string][]string{"test": []string{string(rune(0x19))}, + md: map[string][]string{"test": {string(rune(0x19))}}, want: errors.New("header val has not printable ASCII"), }, { - md: map[string][]string{"test-bin": []string{rune(0x19)}}, + md: map[string][]string{"test-bin": {string(rune(0x19))}}, want: nil, }, } { From 84bbd645547d0f8728b5ecb51aae858843a7b2f0 Mon Sep 17 00:00:00 2001 From: Patrick Date: Tue, 14 Dec 2021 11:51:20 +0800 Subject: [PATCH 11/26] format code --- internal/metadata/metadata.go | 2 +- internal/metadata/metadata_test.go | 16 ++++++++-------- stream.go | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/internal/metadata/metadata.go b/internal/metadata/metadata.go index 02c0dea7331..c19817e41cb 100644 --- a/internal/metadata/metadata.go +++ b/internal/metadata/metadata.go @@ -85,7 +85,7 @@ func Set(addr resolver.Address, md metadata.MD) resolver.Address { func Validate(md metadata.MD) error { for k, vals := range md { // check key, for i that saving a conversion if not using for range - for i := 0; i< len(k); i++ { + for i := 0; i < len(k); i++ { r := k[i] if !(r >= 'a' && r <= 'z') && !(r >= '0' && r <= '9') && r != '.' && r != '-' && r != '_' { return errors.New("header key is not 0-9a-z-_.") diff --git a/internal/metadata/metadata_test.go b/internal/metadata/metadata_test.go index eef4633c4ee..b1363b9e706 100644 --- a/internal/metadata/metadata_test.go +++ b/internal/metadata/metadata_test.go @@ -89,20 +89,20 @@ func TestSet(t *testing.T) { func TestValidate(t *testing.T) { for _, test := range []struct { - md metadata.MD - want error + md metadata.MD + want error }{ { - md: map[string][]string{string(rune(0x19)): {"testVal"}}, - want: errors.New("header key is not 0-9a-z-_."), + md: map[string][]string{string(rune(0x19)): {"testVal"}}, + want: errors.New("header key is not 0-9a-z-_."), }, { - md: map[string][]string{"test": {string(rune(0x19))}}, - want: errors.New("header val has not printable ASCII"), + md: map[string][]string{"test": {string(rune(0x19))}}, + want: errors.New("header val has not printable ASCII"), }, { - md: map[string][]string{"test-bin": {string(rune(0x19))}}, - want: nil, + md: map[string][]string{"test-bin": {string(rune(0x19))}}, + want: nil, }, } { err := Validate(test.md) diff --git a/stream.go b/stream.go index f2f32be7396..ed58fce22cf 100644 --- a/stream.go +++ b/stream.go @@ -165,7 +165,7 @@ func NewClientStream(ctx context.Context, desc *StreamDesc, cc *ClientConn, meth } func newClientStream(ctx context.Context, desc *StreamDesc, cc *ClientConn, method string, opts ...CallOption) (_ ClientStream, err error) { - if md, _ ,ok := metadata.FromOutgoingContextRaw(ctx); ok { + if md, _, ok := metadata.FromOutgoingContextRaw(ctx); ok { if err := imetadata.Validate(md); err != nil { return nil, status.Error(codes.Internal, err.Error()) } From bc980fb431e35553acfc18f0c2bc843f43cd0ed4 Mon Sep 17 00:00:00 2001 From: Patrick Date: Fri, 17 Dec 2021 16:11:28 +0800 Subject: [PATCH 12/26] ignore presudo header and add tests --- internal/metadata/metadata.go | 10 +++-- internal/metadata/metadata_test.go | 4 +- test/metadata_test.go | 72 ++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 5 deletions(-) create mode 100644 test/metadata_test.go diff --git a/internal/metadata/metadata.go b/internal/metadata/metadata.go index c19817e41cb..92d241e42eb 100644 --- a/internal/metadata/metadata.go +++ b/internal/metadata/metadata.go @@ -78,17 +78,21 @@ func Set(addr resolver.Address, md metadata.MD) resolver.Address { // Validate returns an error if the input md contains invalid keys or values. // -// There are check items: +// if header name not presudo-header, there are check items: // - header names contain one or more characters from this set [0-9 a-z _ - .] // - if the header-name ends with a "-bin" suffix, the header-value could contain an arbitrary octet sequence. So no real validation required here. // - if header-name does not end with a "-bin" suffix, header-value should only contain one or more characters from the set ( %x20-%x7E ) which includes space and printable ASCII. func Validate(md metadata.MD) error { for k, vals := range md { + // presudo-header will be ignored + if k[0] == ':' { + continue + } // check key, for i that saving a conversion if not using for range for i := 0; i < len(k); i++ { r := k[i] if !(r >= 'a' && r <= 'z') && !(r >= '0' && r <= '9') && r != '.' && r != '-' && r != '_' { - return errors.New("header key is not 0-9a-z-_.") + return errors.New("header key contains not 0-9a-z-_. characters") } } if strings.HasSuffix(k, "-bin") { @@ -97,7 +101,7 @@ func Validate(md metadata.MD) error { // check value for _, val := range vals { if hasNotPrintable(val) { - return errors.New("header val has not printable ASCII") + return errors.New("header val contains not printable ASCII characters") } } } diff --git a/internal/metadata/metadata_test.go b/internal/metadata/metadata_test.go index b1363b9e706..7f5ea85f3ab 100644 --- a/internal/metadata/metadata_test.go +++ b/internal/metadata/metadata_test.go @@ -94,11 +94,11 @@ func TestValidate(t *testing.T) { }{ { md: map[string][]string{string(rune(0x19)): {"testVal"}}, - want: errors.New("header key is not 0-9a-z-_."), + want: errors.New("header key contains not 0-9a-z-_. characters"), }, { md: map[string][]string{"test": {string(rune(0x19))}}, - want: errors.New("header val has not printable ASCII"), + want: errors.New("header val contains not printable ASCII characters"), }, { md: map[string][]string{"test-bin": {string(rune(0x19))}}, diff --git a/test/metadata_test.go b/test/metadata_test.go new file mode 100644 index 00000000000..57ac27073ee --- /dev/null +++ b/test/metadata_test.go @@ -0,0 +1,72 @@ +/* + * + * Copyright 2014 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package test + +import ( + "context" + "reflect" + "testing" + "time" + + "google.golang.org/grpc/codes" + "google.golang.org/grpc/internal/stubserver" + "google.golang.org/grpc/metadata" + "google.golang.org/grpc/status" + testpb "google.golang.org/grpc/test/grpc_testing" +) + +// TestInvalidMetadata test invalid metadata +func (s) TestInvalidMetadata(t *testing.T) { + + ss := &stubserver.StubServer{ + EmptyCallF: func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error) { + return &testpb.Empty{}, nil + }, + } + if err := ss.Start(nil); err != nil { + t.Fatalf("Error starting endpoint server: %v", err) + } + defer ss.Stop() + + for _, test := range []struct { + md metadata.MD + want error + }{ + { + md: map[string][]string{string(rune(0x19)): {"testVal"}}, + want: status.Error(codes.Internal, "header key contains not 0-9a-z-_. characters"), + }, + { + md: map[string][]string{"test": {string(rune(0x19))}}, + want: status.Error(codes.Internal, "header val contains not printable ASCII characters"), + }, + { + md: map[string][]string{"test-bin": {string(rune(0x19))}}, + want: nil, + }, + } { + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + ctx = metadata.NewOutgoingContext(ctx, test.md) + if _, err := ss.Client.EmptyCall(ctx, &testpb.Empty{}); !reflect.DeepEqual(test.want, err) { + t.Fatalf("call ss.Client.EmptyCall() validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) + } + } +} From 60aca707967f46aa38573446991ff7b650788225 Mon Sep 17 00:00:00 2001 From: Patrick Date: Fri, 7 Jan 2022 15:08:33 +0800 Subject: [PATCH 13/26] fix problems --- internal/metadata/metadata.go | 4 +-- test/metadata_test.go | 53 ++++++++++++++++++++++++++--------- 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/internal/metadata/metadata.go b/internal/metadata/metadata.go index 92d241e42eb..741479837b4 100644 --- a/internal/metadata/metadata.go +++ b/internal/metadata/metadata.go @@ -78,13 +78,13 @@ func Set(addr resolver.Address, md metadata.MD) resolver.Address { // Validate returns an error if the input md contains invalid keys or values. // -// if header name not presudo-header, there are check items: +// if the header is not pseudo-header, there are check items: // - header names contain one or more characters from this set [0-9 a-z _ - .] // - if the header-name ends with a "-bin" suffix, the header-value could contain an arbitrary octet sequence. So no real validation required here. // - if header-name does not end with a "-bin" suffix, header-value should only contain one or more characters from the set ( %x20-%x7E ) which includes space and printable ASCII. func Validate(md metadata.MD) error { for k, vals := range md { - // presudo-header will be ignored + // pseudo-header will be ignored if k[0] == ':' { continue } diff --git a/test/metadata_test.go b/test/metadata_test.go index 57ac27073ee..755abd6911f 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -1,6 +1,6 @@ /* * - * Copyright 2014 gRPC authors. + * Copyright 2022 gRPC authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -34,17 +34,7 @@ import ( // TestInvalidMetadata test invalid metadata func (s) TestInvalidMetadata(t *testing.T) { - ss := &stubserver.StubServer{ - EmptyCallF: func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error) { - return &testpb.Empty{}, nil - }, - } - if err := ss.Start(nil); err != nil { - t.Fatalf("Error starting endpoint server: %v", err) - } - defer ss.Stop() - - for _, test := range []struct { + tests := []struct { md metadata.MD want error }{ @@ -60,7 +50,19 @@ func (s) TestInvalidMetadata(t *testing.T) { md: map[string][]string{"test-bin": {string(rune(0x19))}}, want: nil, }, - } { + } + + ss := &stubserver.StubServer{ + EmptyCallF: func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error) { + return &testpb.Empty{}, nil + }, + } + if err := ss.Start(nil); err != nil { + t.Fatalf("Error starting ss endpoint server: %v", err) + } + defer ss.Stop() + + for _, test := range tests { ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) defer cancel() @@ -69,4 +71,29 @@ func (s) TestInvalidMetadata(t *testing.T) { t.Fatalf("call ss.Client.EmptyCall() validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) } } + + ss2 := &stubserver.StubServer{ + FullDuplexCallF: func(stream testpb.TestService_FullDuplexCallServer) error { + + for _, test := range tests { + if err := stream.SendHeader(test.md); !reflect.DeepEqual(test.want, err) { + t.Fatalf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) + } + stream.SetTrailer(test.md) + } + if err := stream.Send(nil); err != nil { + t.Fatalf("call stream.Send(nil) will success but got err :%v", err) + } + return nil + }, + } + if err := ss2.Start(nil); err != nil { + t.Fatalf("Error starting ss2 endpoint server: %v", err) + } + defer ss2.Stop() + + if _, err := ss2.Client.FullDuplexCall(context.Background()); err != nil { + t.Fatalf("call ss2.Client.FullDuplexCall(context.Background()) will success but got err :%v", err) + } + } From 82240b2551a9d3ff5d784903fba105563765ca78 Mon Sep 17 00:00:00 2001 From: Patrick Date: Fri, 7 Jan 2022 17:11:10 +0800 Subject: [PATCH 14/26] fix test problem --- stream.go | 4 +-- test/metadata_test.go | 60 ++++++++++++++++++++++++++++--------------- 2 files changed, 41 insertions(+), 23 deletions(-) diff --git a/stream.go b/stream.go index ed58fce22cf..a9a4439ce59 100644 --- a/stream.go +++ b/stream.go @@ -1454,7 +1454,7 @@ func (ss *serverStream) SetHeader(md metadata.MD) error { } err := imetadata.Validate(md) if err != nil { - return err + return status.Error(codes.Internal, err.Error()) } return ss.s.SetHeader(md) } @@ -1462,7 +1462,7 @@ func (ss *serverStream) SetHeader(md metadata.MD) error { func (ss *serverStream) SendHeader(md metadata.MD) error { err := imetadata.Validate(md) if err != nil { - return err + return status.Error(codes.Internal, err.Error()) } err = ss.t.WriteHeader(ss.s, md) diff --git a/test/metadata_test.go b/test/metadata_test.go index 755abd6911f..bdfee928056 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -20,10 +20,12 @@ package test import ( "context" + "io" "reflect" "testing" "time" + "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/internal/stubserver" "google.golang.org/grpc/metadata" @@ -56,6 +58,30 @@ func (s) TestInvalidMetadata(t *testing.T) { EmptyCallF: func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error) { return &testpb.Empty{}, nil }, + FullDuplexCallF: func(stream testpb.TestService_FullDuplexCallServer) error { + for { + _, err := stream.Recv() + if err == io.EOF { + return nil + } + if err != nil { + return err + } + for _, test := range tests { + if err := stream.SetHeader(test.md); !reflect.DeepEqual(test.want, err) { + t.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) + } + if err := stream.SendHeader(test.md); !reflect.DeepEqual(test.want, err) { + t.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) + } + stream.SetTrailer(test.md) + } + err = stream.Send(&testpb.StreamingOutputCallResponse{}) + if err != nil { + return err + } + } + }, } if err := ss.Start(nil); err != nil { t.Fatalf("Error starting ss endpoint server: %v", err) @@ -72,28 +98,20 @@ func (s) TestInvalidMetadata(t *testing.T) { } } - ss2 := &stubserver.StubServer{ - FullDuplexCallF: func(stream testpb.TestService_FullDuplexCallServer) error { - - for _, test := range tests { - if err := stream.SendHeader(test.md); !reflect.DeepEqual(test.want, err) { - t.Fatalf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) - } - stream.SetTrailer(test.md) - } - if err := stream.Send(nil); err != nil { - t.Fatalf("call stream.Send(nil) will success but got err :%v", err) - } - return nil - }, + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + stream, err := ss.Client.FullDuplexCall(ctx, grpc.WaitForReady(true)) + defer cancel() + if err != nil { + t.Fatalf("call ss.Client.FullDuplexCall(context.Background()) will success but got err :%v", err) } - if err := ss2.Start(nil); err != nil { - t.Fatalf("Error starting ss2 endpoint server: %v", err) + if err := stream.Send(&testpb.StreamingOutputCallRequest{}); err != nil { + t.Fatalf("call ss.Client stream Send(nil) will success but got err :%v", err) } - defer ss2.Stop() - - if _, err := ss2.Client.FullDuplexCall(context.Background()); err != nil { - t.Fatalf("call ss2.Client.FullDuplexCall(context.Background()) will success but got err :%v", err) + if _, err := stream.Header(); err != nil { + t.Fatalf("call ss.Client stream Send(nil) will success but got err :%v", err) } - + if _, err := stream.Recv(); err != nil { + t.Fatalf("stream.Recv() = _, %v", err) + } + stream.CloseSend() } From 4bee0159a8f31a7d7f2ba71041aa2c5905190cbd Mon Sep 17 00:00:00 2001 From: Patrick Date: Thu, 13 Jan 2022 11:48:38 +0800 Subject: [PATCH 15/26] format code --- test/metadata_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/metadata_test.go b/test/metadata_test.go index bdfee928056..2a857dbc39a 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -105,13 +105,13 @@ func (s) TestInvalidMetadata(t *testing.T) { t.Fatalf("call ss.Client.FullDuplexCall(context.Background()) will success but got err :%v", err) } if err := stream.Send(&testpb.StreamingOutputCallRequest{}); err != nil { - t.Fatalf("call ss.Client stream Send(nil) will success but got err :%v", err) + t.Fatalf("call ss.Client stream Send(nil) will success but got err :%v", err) } if _, err := stream.Header(); err != nil { - t.Fatalf("call ss.Client stream Send(nil) will success but got err :%v", err) + t.Fatalf("call ss.Client stream Send(nil) will success but got err :%v", err) } if _, err := stream.Recv(); err != nil { - t.Fatalf("stream.Recv() = _, %v", err) + t.Fatalf("stream.Recv() = _, %v", err) } stream.CloseSend() } From f04f60cc718b389f205387f3909ef8db431764be Mon Sep 17 00:00:00 2001 From: Patrick Date: Sun, 16 Jan 2022 13:59:57 +0800 Subject: [PATCH 16/26] fix review problem --- internal/metadata/metadata.go | 13 ++++----- test/metadata_test.go | 51 ++++++++++++++++++----------------- 2 files changed, 34 insertions(+), 30 deletions(-) diff --git a/internal/metadata/metadata.go b/internal/metadata/metadata.go index 741479837b4..8ae2225791c 100644 --- a/internal/metadata/metadata.go +++ b/internal/metadata/metadata.go @@ -23,6 +23,7 @@ package metadata import ( "errors" + "fmt" "strings" "google.golang.org/grpc/metadata" @@ -78,10 +79,10 @@ func Set(addr resolver.Address, md metadata.MD) resolver.Address { // Validate returns an error if the input md contains invalid keys or values. // -// if the header is not pseudo-header, there are check items: -// - header names contain one or more characters from this set [0-9 a-z _ - .] -// - if the header-name ends with a "-bin" suffix, the header-value could contain an arbitrary octet sequence. So no real validation required here. -// - if header-name does not end with a "-bin" suffix, header-value should only contain one or more characters from the set ( %x20-%x7E ) which includes space and printable ASCII. +// If the header is not a pseudo-header, the following items are checked: +// - header names must contain one or more characters from this set [0-9 a-z _ - .]. +// - if the header-name ends with a "-bin" suffix, no validation of the header value is performed. +// - otherwise, the header value must contain one or more characters from the set [%x20-%x7E]. func Validate(md metadata.MD) error { for k, vals := range md { // pseudo-header will be ignored @@ -92,7 +93,7 @@ func Validate(md metadata.MD) error { for i := 0; i < len(k); i++ { r := k[i] if !(r >= 'a' && r <= 'z') && !(r >= '0' && r <= '9') && r != '.' && r != '-' && r != '_' { - return errors.New("header key contains not 0-9a-z-_. characters") + return fmt.Errorf("header key %q contains not 0-9a-z-_. characters", k) } } if strings.HasSuffix(k, "-bin") { @@ -101,7 +102,7 @@ func Validate(md metadata.MD) error { // check value for _, val := range vals { if hasNotPrintable(val) { - return errors.New("header val contains not printable ASCII characters") + return fmt.Errorf("header key %q contains value non-printable ASCII characters", k) } } } diff --git a/test/metadata_test.go b/test/metadata_test.go index 2a857dbc39a..f1f542c5adc 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -33,7 +33,6 @@ import ( testpb "google.golang.org/grpc/test/grpc_testing" ) -// TestInvalidMetadata test invalid metadata func (s) TestInvalidMetadata(t *testing.T) { tests := []struct { @@ -52,8 +51,13 @@ func (s) TestInvalidMetadata(t *testing.T) { md: map[string][]string{"test-bin": {string(rune(0x19))}}, want: nil, }, + { + md: map[string][]string{"test": {"value"}}, + want: nil, + }, } + testNum := 0 ss := &stubserver.StubServer{ EmptyCallF: func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error) { return &testpb.Empty{}, nil @@ -67,15 +71,15 @@ func (s) TestInvalidMetadata(t *testing.T) { if err != nil { return err } - for _, test := range tests { - if err := stream.SetHeader(test.md); !reflect.DeepEqual(test.want, err) { - t.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) - } - if err := stream.SendHeader(test.md); !reflect.DeepEqual(test.want, err) { - t.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) - } - stream.SetTrailer(test.md) + test := tests[testNum] + testNum = testNum + 1 + if err := stream.SetHeader(test.md); !reflect.DeepEqual(test.want, err) { + t.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) } + if err := stream.SendHeader(test.md); !reflect.DeepEqual(test.want, err) { + t.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) + } + stream.SetTrailer(test.md) err = stream.Send(&testpb.StreamingOutputCallResponse{}) if err != nil { return err @@ -98,20 +102,19 @@ func (s) TestInvalidMetadata(t *testing.T) { } } - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) - stream, err := ss.Client.FullDuplexCall(ctx, grpc.WaitForReady(true)) - defer cancel() - if err != nil { - t.Fatalf("call ss.Client.FullDuplexCall(context.Background()) will success but got err :%v", err) - } - if err := stream.Send(&testpb.StreamingOutputCallRequest{}); err != nil { - t.Fatalf("call ss.Client stream Send(nil) will success but got err :%v", err) - } - if _, err := stream.Header(); err != nil { - t.Fatalf("call ss.Client stream Send(nil) will success but got err :%v", err) - } - if _, err := stream.Recv(); err != nil { - t.Fatalf("stream.Recv() = _, %v", err) + for _, _ = range tests { + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + stream, err := ss.Client.FullDuplexCall(ctx, grpc.WaitForReady(true)) + defer cancel() + if err != nil { + t.Fatalf("call ss.Client.FullDuplexCall(context.Background()) will success but got err :%v", err) + } + if err := stream.Send(&testpb.StreamingOutputCallRequest{}); err != nil { + t.Fatalf("call ss.Client stream Send(nil) will success but got err :%v", err) + } + if _, err := stream.Recv(); err != nil { + t.Fatalf("stream.Recv() = _, %v", err) + } + stream.CloseSend() } - stream.CloseSend() } From a5d18dd0e5b352949bf5d099c82c3dea9429fed0 Mon Sep 17 00:00:00 2001 From: Patrick Date: Sun, 16 Jan 2022 14:00:46 +0800 Subject: [PATCH 17/26] fix format --- test/metadata_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/metadata_test.go b/test/metadata_test.go index f1f542c5adc..49c95d74e4c 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -102,7 +102,7 @@ func (s) TestInvalidMetadata(t *testing.T) { } } - for _, _ = range tests { + for range tests { ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) stream, err := ss.Client.FullDuplexCall(ctx, grpc.WaitForReady(true)) defer cancel() From cb062d0dd5894eeba55759d0e3d9edefe75645d9 Mon Sep 17 00:00:00 2001 From: Patrick Date: Wed, 19 Jan 2022 11:01:15 +0800 Subject: [PATCH 18/26] delete useless import and fix tests --- internal/metadata/metadata.go | 1 - internal/metadata/metadata_test.go | 4 ++-- test/metadata_test.go | 4 ++-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/internal/metadata/metadata.go b/internal/metadata/metadata.go index 8ae2225791c..34b42940490 100644 --- a/internal/metadata/metadata.go +++ b/internal/metadata/metadata.go @@ -22,7 +22,6 @@ package metadata import ( - "errors" "fmt" "strings" diff --git a/internal/metadata/metadata_test.go b/internal/metadata/metadata_test.go index 7f5ea85f3ab..54a947bcf14 100644 --- a/internal/metadata/metadata_test.go +++ b/internal/metadata/metadata_test.go @@ -94,11 +94,11 @@ func TestValidate(t *testing.T) { }{ { md: map[string][]string{string(rune(0x19)): {"testVal"}}, - want: errors.New("header key contains not 0-9a-z-_. characters"), + want: errors.New("header key \"\\x19\" contains not 0-9a-z-_. characters"), }, { md: map[string][]string{"test": {string(rune(0x19))}}, - want: errors.New("header val contains not printable ASCII characters"), + want: errors.New("header key \"test\" contains value non-printable ASCII characters"), }, { md: map[string][]string{"test-bin": {string(rune(0x19))}}, diff --git a/test/metadata_test.go b/test/metadata_test.go index 49c95d74e4c..f3cc1d756e2 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -41,11 +41,11 @@ func (s) TestInvalidMetadata(t *testing.T) { }{ { md: map[string][]string{string(rune(0x19)): {"testVal"}}, - want: status.Error(codes.Internal, "header key contains not 0-9a-z-_. characters"), + want: status.Error(codes.Internal, "header key \"\\x19\" contains not 0-9a-z-_. characters"), }, { md: map[string][]string{"test": {string(rune(0x19))}}, - want: status.Error(codes.Internal, "header val contains not printable ASCII characters"), + want: status.Error(codes.Internal, "header key \"test\" contains value non-printable ASCII characters"), }, { md: map[string][]string{"test-bin": {string(rune(0x19))}}, From fad092e6082e7af0ea4e9e9ae5208805c7b145bf Mon Sep 17 00:00:00 2001 From: Patrick Date: Thu, 20 Jan 2022 15:50:13 +0800 Subject: [PATCH 19/26] fix review problem --- internal/metadata/metadata.go | 4 ++-- internal/metadata/metadata_test.go | 4 ++-- stream.go | 3 ++- test/metadata_test.go | 4 ++-- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/internal/metadata/metadata.go b/internal/metadata/metadata.go index 34b42940490..b2980f8ac44 100644 --- a/internal/metadata/metadata.go +++ b/internal/metadata/metadata.go @@ -92,7 +92,7 @@ func Validate(md metadata.MD) error { for i := 0; i < len(k); i++ { r := k[i] if !(r >= 'a' && r <= 'z') && !(r >= '0' && r <= '9') && r != '.' && r != '-' && r != '_' { - return fmt.Errorf("header key %q contains not 0-9a-z-_. characters", k) + return fmt.Errorf("header key %q contains illegal characters not in [0-9a-z-_.]", k) } } if strings.HasSuffix(k, "-bin") { @@ -101,7 +101,7 @@ func Validate(md metadata.MD) error { // check value for _, val := range vals { if hasNotPrintable(val) { - return fmt.Errorf("header key %q contains value non-printable ASCII characters", k) + return fmt.Errorf("header key %q contains value with non-printable ASCII characters", k) } } } diff --git a/internal/metadata/metadata_test.go b/internal/metadata/metadata_test.go index 54a947bcf14..80f1a44bb6a 100644 --- a/internal/metadata/metadata_test.go +++ b/internal/metadata/metadata_test.go @@ -94,11 +94,11 @@ func TestValidate(t *testing.T) { }{ { md: map[string][]string{string(rune(0x19)): {"testVal"}}, - want: errors.New("header key \"\\x19\" contains not 0-9a-z-_. characters"), + want: errors.New("header key \"\\x19\" contains illegal characters not in [0-9a-z-_.]"), }, { md: map[string][]string{"test": {string(rune(0x19))}}, - want: errors.New("header key \"test\" contains value non-printable ASCII characters"), + want: errors.New("header key \"test\" contains value with non-printable ASCII characters"), }, { md: map[string][]string{"test-bin": {string(rune(0x19))}}, diff --git a/stream.go b/stream.go index a9a4439ce59..750bfae57f7 100644 --- a/stream.go +++ b/stream.go @@ -1480,7 +1480,8 @@ func (ss *serverStream) SetTrailer(md metadata.MD) { if md.Len() == 0 { return } - if imetadata.Validate(md) != nil { + if err := imetadata.Validate(md); err != nil { + logger.Errorf("stream: failed to set trailer, err: %v", err) return } ss.s.SetTrailer(md) diff --git a/test/metadata_test.go b/test/metadata_test.go index f3cc1d756e2..d3a260e0af4 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -41,11 +41,11 @@ func (s) TestInvalidMetadata(t *testing.T) { }{ { md: map[string][]string{string(rune(0x19)): {"testVal"}}, - want: status.Error(codes.Internal, "header key \"\\x19\" contains not 0-9a-z-_. characters"), + want: status.Error(codes.Internal, "header key \"\\x19\" contains illegal characters not in [0-9a-z-_.]"), }, { md: map[string][]string{"test": {string(rune(0x19))}}, - want: status.Error(codes.Internal, "header key \"test\" contains value non-printable ASCII characters"), + want: status.Error(codes.Internal, "header key \"test\" contains value with non-printable ASCII characters"), }, { md: map[string][]string{"test-bin": {string(rune(0x19))}}, From 9e3a801897eb29796e5a4a56b1a983bc33e9dfd5 Mon Sep 17 00:00:00 2001 From: Patrick Date: Mon, 24 Jan 2022 17:13:54 +0800 Subject: [PATCH 20/26] fix tests problem --- test/metadata_test.go | 47 ++++++++++++++++++------------------------- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/test/metadata_test.go b/test/metadata_test.go index d3a260e0af4..02dc91bdb5e 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -20,6 +20,7 @@ package test import ( "context" + "fmt" "io" "reflect" "testing" @@ -63,28 +64,20 @@ func (s) TestInvalidMetadata(t *testing.T) { return &testpb.Empty{}, nil }, FullDuplexCallF: func(stream testpb.TestService_FullDuplexCallServer) error { - for { - _, err := stream.Recv() - if err == io.EOF { - return nil - } - if err != nil { - return err - } - test := tests[testNum] - testNum = testNum + 1 - if err := stream.SetHeader(test.md); !reflect.DeepEqual(test.want, err) { - t.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) - } - if err := stream.SendHeader(test.md); !reflect.DeepEqual(test.want, err) { - t.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) - } - stream.SetTrailer(test.md) - err = stream.Send(&testpb.StreamingOutputCallResponse{}) - if err != nil { - return err - } + _, err := stream.Recv() + if err != nil { + return err } + test := tests[testNum] + testNum = testNum + 1 + if err := stream.SetHeader(test.md); !reflect.DeepEqual(test.want, err) { + return fmt.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) + } + if err := stream.SendHeader(test.md); !reflect.DeepEqual(test.want, err) { + return fmt.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) + } + stream.SetTrailer(test.md) + return nil }, } if err := ss.Start(nil); err != nil { @@ -98,23 +91,23 @@ func (s) TestInvalidMetadata(t *testing.T) { ctx = metadata.NewOutgoingContext(ctx, test.md) if _, err := ss.Client.EmptyCall(ctx, &testpb.Empty{}); !reflect.DeepEqual(test.want, err) { - t.Fatalf("call ss.Client.EmptyCall() validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) + t.Errorf("call ss.Client.EmptyCall() validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) } } + // call the stream server's api to drive the server-side unit testing for range tests { ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) stream, err := ss.Client.FullDuplexCall(ctx, grpc.WaitForReady(true)) defer cancel() if err != nil { - t.Fatalf("call ss.Client.FullDuplexCall(context.Background()) will success but got err :%v", err) + t.Errorf("call ss.Client.FullDuplexCall(context.Background()) will success but got err :%v", err) } if err := stream.Send(&testpb.StreamingOutputCallRequest{}); err != nil { - t.Fatalf("call ss.Client stream Send(nil) will success but got err :%v", err) + t.Errorf("call ss.Client stream Send(nil) will success but got err :%v", err) } - if _, err := stream.Recv(); err != nil { - t.Fatalf("stream.Recv() = _, %v", err) + if _, err := stream.Recv(); err != io.EOF { + t.Errorf("stream.Recv() = _, %v", err) } - stream.CloseSend() } } From 4084375a7cbdb8acf0ddf59fd7da8d796dc8a4e8 Mon Sep 17 00:00:00 2001 From: Patrick Date: Mon, 24 Jan 2022 17:20:38 +0800 Subject: [PATCH 21/26] fix problem --- stream.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/stream.go b/stream.go index 750bfae57f7..5b8b34a12f3 100644 --- a/stream.go +++ b/stream.go @@ -1481,8 +1481,7 @@ func (ss *serverStream) SetTrailer(md metadata.MD) { return } if err := imetadata.Validate(md); err != nil { - logger.Errorf("stream: failed to set trailer, err: %v", err) - return + logger.Errorf("stream: failed to validate md when setting trailer, err: %v", err) } ss.s.SetTrailer(md) } From 145bef2dba4a84fc13a5a0da636aaaba2b6c44c5 Mon Sep 17 00:00:00 2001 From: Patrick Date: Mon, 24 Jan 2022 17:46:30 +0800 Subject: [PATCH 22/26] format code --- test/metadata_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/metadata_test.go b/test/metadata_test.go index 02dc91bdb5e..623415cad2f 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -39,22 +39,27 @@ func (s) TestInvalidMetadata(t *testing.T) { tests := []struct { md metadata.MD want error + recv error }{ { md: map[string][]string{string(rune(0x19)): {"testVal"}}, want: status.Error(codes.Internal, "header key \"\\x19\" contains illegal characters not in [0-9a-z-_.]"), + recv: status.Error(codes.Internal, " invalid header field name \"\x19\""), }, { md: map[string][]string{"test": {string(rune(0x19))}}, want: status.Error(codes.Internal, "header key \"test\" contains value with non-printable ASCII characters"), + recv: status.Error(codes.Internal, " invalid header field value \"\x19\""), }, { md: map[string][]string{"test-bin": {string(rune(0x19))}}, want: nil, + recv: nil, }, { md: map[string][]string{"test": {"value"}}, want: nil, + recv: nil, }, } @@ -96,7 +101,7 @@ func (s) TestInvalidMetadata(t *testing.T) { } // call the stream server's api to drive the server-side unit testing - for range tests { + for _, test := range tests { ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) stream, err := ss.Client.FullDuplexCall(ctx, grpc.WaitForReady(true)) defer cancel() @@ -106,7 +111,7 @@ func (s) TestInvalidMetadata(t *testing.T) { if err := stream.Send(&testpb.StreamingOutputCallRequest{}); err != nil { t.Errorf("call ss.Client stream Send(nil) will success but got err :%v", err) } - if _, err := stream.Recv(); err != io.EOF { + if _, err := stream.Recv(); !reflect.DeepEqual(test.recv, err) && err != io.EOF { t.Errorf("stream.Recv() = _, %v", err) } } From 881ab45823b04534d59ce16c42b67b560426f2d1 Mon Sep 17 00:00:00 2001 From: Patrick Date: Mon, 24 Jan 2022 18:04:34 +0800 Subject: [PATCH 23/26] update tests --- test/metadata_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/metadata_test.go b/test/metadata_test.go index 623415cad2f..5ac21e42283 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -44,22 +44,22 @@ func (s) TestInvalidMetadata(t *testing.T) { { md: map[string][]string{string(rune(0x19)): {"testVal"}}, want: status.Error(codes.Internal, "header key \"\\x19\" contains illegal characters not in [0-9a-z-_.]"), - recv: status.Error(codes.Internal, " invalid header field name \"\x19\""), + recv: status.Error(codes.Internal, "invalid header field name \"\\x19\""), }, { md: map[string][]string{"test": {string(rune(0x19))}}, want: status.Error(codes.Internal, "header key \"test\" contains value with non-printable ASCII characters"), - recv: status.Error(codes.Internal, " invalid header field value \"\x19\""), + recv: status.Error(codes.Internal, "invalid header field value \"\\x19\""), }, { md: map[string][]string{"test-bin": {string(rune(0x19))}}, want: nil, - recv: nil, + recv: io.EOF, }, { md: map[string][]string{"test": {"value"}}, want: nil, - recv: nil, + recv: io.EOF, }, } @@ -111,8 +111,8 @@ func (s) TestInvalidMetadata(t *testing.T) { if err := stream.Send(&testpb.StreamingOutputCallRequest{}); err != nil { t.Errorf("call ss.Client stream Send(nil) will success but got err :%v", err) } - if _, err := stream.Recv(); !reflect.DeepEqual(test.recv, err) && err != io.EOF { - t.Errorf("stream.Recv() = _, %v", err) + if _, err := stream.Recv(); !reflect.DeepEqual(test.recv, err) { + t.Errorf("stream.Recv() = _, get err :%v, want err :%v", err, test.recv) } } } From 47c552d132d87e09dcc707dfe7ae1f0fd2749180 Mon Sep 17 00:00:00 2001 From: Patrick Date: Fri, 4 Feb 2022 16:54:03 +0800 Subject: [PATCH 24/26] fix review problem --- test/metadata_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/metadata_test.go b/test/metadata_test.go index 5ac21e42283..0c3df7a8fb3 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -26,7 +26,6 @@ import ( "testing" "time" - "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/internal/stubserver" "google.golang.org/grpc/metadata" @@ -74,7 +73,7 @@ func (s) TestInvalidMetadata(t *testing.T) { return err } test := tests[testNum] - testNum = testNum + 1 + testNum ++ if err := stream.SetHeader(test.md); !reflect.DeepEqual(test.want, err) { return fmt.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) } @@ -103,10 +102,11 @@ func (s) TestInvalidMetadata(t *testing.T) { // call the stream server's api to drive the server-side unit testing for _, test := range tests { ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) - stream, err := ss.Client.FullDuplexCall(ctx, grpc.WaitForReady(true)) + stream, err := ss.Client.FullDuplexCall(ctx) defer cancel() if err != nil { t.Errorf("call ss.Client.FullDuplexCall(context.Background()) will success but got err :%v", err) + continue } if err := stream.Send(&testpb.StreamingOutputCallRequest{}); err != nil { t.Errorf("call ss.Client stream Send(nil) will success but got err :%v", err) From 30b92a3443f1d17bbea515a001ee24d242e07f54 Mon Sep 17 00:00:00 2001 From: Patrick Date: Tue, 22 Feb 2022 10:05:41 +0800 Subject: [PATCH 25/26] format --- test/metadata_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/metadata_test.go b/test/metadata_test.go index 0c3df7a8fb3..6001f529992 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -73,7 +73,7 @@ func (s) TestInvalidMetadata(t *testing.T) { return err } test := tests[testNum] - testNum ++ + testNum++ if err := stream.SetHeader(test.md); !reflect.DeepEqual(test.want, err) { return fmt.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) } From 6674adc9df0dd13a760d0e2258c3916b4663a9ce Mon Sep 17 00:00:00 2001 From: Patrick Date: Wed, 23 Feb 2022 15:40:10 +0800 Subject: [PATCH 26/26] expect error log --- test/metadata_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/metadata_test.go b/test/metadata_test.go index 6001f529992..e3da918fc72 100644 --- a/test/metadata_test.go +++ b/test/metadata_test.go @@ -27,6 +27,7 @@ import ( "time" "google.golang.org/grpc/codes" + "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/internal/stubserver" "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" @@ -34,6 +35,7 @@ import ( ) func (s) TestInvalidMetadata(t *testing.T) { + grpctest.TLogger.ExpectErrorN("stream: failed to validate md when setting trailer", 2) tests := []struct { md metadata.MD