Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panic on non-utf-8 status messaage #2078

Closed
euroelessar opened this issue May 15, 2018 · 4 comments · Fixed by #2109 or #2134
Closed

Panic on non-utf-8 status messaage #2078

euroelessar opened this issue May 15, 2018 · 4 comments · Fixed by #2109 or #2134
Assignees

Comments

@euroelessar
Copy link

Please answer these questions before submitting your issue.

What version of gRPC are you using?

grpc-go 1.11

What version of Go are you using (go version)?

go version go1.8.4 linux/amd64

What operating system (Linux, Windows, …) and version?

Linux

What did you do?

Server returns status with message which is invalid utf-8 sequence first without details and after that with some details.

What did you expect to see?

Consistent behavior, for example sanitizing message to ensure that it's valid utf-8.

What did you see instead?

Returning status with non-utf-8 message but without details is silently encoded by server and is sent to client. Resulting status-message http2 field is not valid escaped utf-8 sequence in violation of http2 protocol spec for grpc.

Returning status with non-utf-8 message and with at least one detail causes panic on a server.

@menghanl
Copy link
Contributor

Can you also provide the message you are trying to return in both cases? A code snippet to create the status in both cases would also be helpful.

Resulting status-message http2 field is not valid escaped utf-8 sequence in violation of http2 protocol spec for grpc.

IIUC, encoded messages should all be ASCII. It's not clear to me how this will result in violation of http2 protocol. An example would help here.

@euroelessar
Copy link
Author

I've used string([]byte{0xff, 0xfe, 0xfd}) as a message, issue can be reproduced by altering SayHello method of greeter server from grpc-go examples: https://gist.github.com/euroelessar/dde1d70cf8e8fe8dd27a81d6b2b9025f

Here is a panic stack trace caused by non-utf-8 message:

panic: proto: invalid UTF-8 string

goroutine 10 [running]:
google.golang.org/grpc/transport.(*http2Server).WriteStatus(0xc420172000, 0xc4201520f0, 0xc4201be018, 0xc420184001, 0xc420182070)
	/Users/elessar/go/src/google.golang.org/grpc/transport/http2_server.go:763 +0x141c
google.golang.org/grpc.(*Server).processUnaryRPC(0xc420154000, 0x1742a60, 0xc420172000, 0xc4201520f0, 0xc42007dc80, 0x17752a0, 0x0, 0x0, 0x0)
	/Users/elessar/go/src/google.golang.org/grpc/server.go:1023 +0xe65
google.golang.org/grpc.(*Server).handleStream(0xc420154000, 0x1742a60, 0xc420172000, 0xc4201520f0, 0x0)
	/Users/elessar/go/src/google.golang.org/grpc/server.go:1249 +0x1528
google.golang.org/grpc.(*Server).serveStreams.func1.1(0xc420018750, 0xc420154000, 0x1742a60, 0xc420172000, 0xc4201520f0)
	/Users/elessar/go/src/google.golang.org/grpc/server.go:680 +0x9f
created by google.golang.org/grpc.(*Server).serveStreams.func1
	/Users/elessar/go/src/google.golang.org/grpc/server.go:678 +0xa1

IIUC, encoded messages should all be ASCII. It's not clear to me how this will result in violation of http2 protocol. An example would help here.

gRPC HTTP2 protocol is a bit more precise:

The value portion of Status-Message is conceptually a Unicode string description of the error, physically encoded as UTF-8 followed by percent-encoding.

But grpc-go doesn't perform any validation of message returned by application handler and therefore sends ASCII encoded string followed by percent encoding which is not always a valid UTF-8 string.

@menghanl
Copy link
Contributor

I see. There are two problems here:

  1. Missing a check of invalid utf-8 chars when creating the status (e.g. status.New())

  2. In the case of status with details, the panic caused by an error returned by proto library

For problem 1, we could add a check for user input, and strip invalid utf-8 chars (by replacing them with Unicode replacement character). The same approach should happen on the receiving side, too.

Problem 2 won't be a problem in the invalid utf-8 cases after we solve problem 1.
I still agree that panic is not a good idea here, but we didn't want to silently ignore the error. There should be another way to notify the user of the marshal error.

@dfawley dfawley added P1 and removed P2 labels May 21, 2018
menghanl added a commit that referenced this issue Jun 6, 2018
fixes #2078

A status with invalid utf-8 characters could still be created, but invalid characters will be replaced with [Unicode replacement character](https://en.wikipedia.org/wiki/Specials_(Unicode_block)#Replacement_character) before being sent out. Those bytes will still be percent encoded.

All details added to this invalid status will be dropped.
@menghanl
Copy link
Contributor

menghanl commented Jun 6, 2018

fix reverted in #2127

@menghanl menghanl reopened this Jun 6, 2018
menghanl added a commit to menghanl/grpc-go that referenced this issue Jun 8, 2018
fixes grpc#2078

A status with invalid utf-8 characters could still be created, but invalid characters will be replaced with [Unicode replacement character](https://en.wikipedia.org/wiki/Specials_(Unicode_block)#Replacement_character) before being sent out. Those bytes will still be percent encoded.

All details added to this invalid status will be dropped.
menghanl added a commit that referenced this issue Jun 14, 2018
fixes #2078

A status with invalid utf-8 characters could still be created, but invalid characters will be replaced with [Unicode replacement character](https://en.wikipedia.org/wiki/Specials_(Unicode_block)#Replacement_character) before being sent out. Those bytes will still be percent encoded.

All details added to this invalid status will be dropped.
@lock lock bot locked as resolved and limited conversation to collaborators Dec 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants