Get empty response when compress enabled and maxReceiveMessageSize be maxInt64

[lysu]

NOTE: if you are reporting is a potential security vulnerability or a crash, please follow our CVE process at https://github.com/grpc/proposal/blob/master/P4-grpc-cve-process.md instead of filing an issue here.

Please see the FAQ in our main README.md, then answer the questions below before submitting your issue.

What version of gRPC are you using?

v1.27.1

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

go.1.16

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

Linux

What did you do?

If possible, provide a recipe for reproducing the error.

grpc.DialContext with two callOptions:

• grpc.UseCompressor("gzip") to enable gzip
• grpc.MaxCallRecvMsgSize(math.MaxInt64) to set maxReceiveMessageSize as maxInt64

call remote service and wait for its response

it can be simple reproduce by https://github.com/lysu/grpc-go/blob/b93ee57c403c18340a91a6c718c0a647af23c1ad/examples/helloworld/greeter_client/main.go#L42

What did you expect to see?

remote service will give a compressed response, we should get the right decompressed response.

What did you see instead?

get empty response

[lysu]

the question seems to be caused by

https://github.com/grpc/grpc-go/blob/master/rpc_util.go#L744-L745

bytesRead, err := buf.ReadFrom(io.LimitReader(dcReader, int64(maxReceiveMessageSize)+1))

will make LimitReader.N be negative value and return nothing

but I'm not sure how to fix those bound condition, PTAL thx

[easwars]

Thanks @lysu for reporting the problem and providing a way to reproduce it.

I was able to reproduce it with the the helloworld example as suggested by you. Also, I can confirm that the following ways of creating the ClientConn work:

conn, err := grpc.Dial(address, 
  grpc.WithInsecure(),
  grpc.WithBlock(),
  grpc.WithDefaultCallOptions(grpc.UseCompressor("gzip")))

conn, err := grpc.Dial(address,
  grpc.WithInsecure(),
  grpc.WithBlock(),
  grpc.WithDefaultCallOptions(grpc.UseCompressor("gzip"), grpc.MaxCallRecvMsgSize(math.MaxInt64-1)))

It's only when math.MaxInt64 is used as the maxCallRecvMsgSize that things don't work:

conn, err := grpc.Dial(address,
  grpc.WithInsecure(),
  grpc.WithBlock(),
  grpc.WithDefaultCallOptions(grpc.UseCompressor("gzip"), grpc.MaxCallRecvMsgSize(math.MaxInt64-1)))

This clearly looks like a bug on our side. We will look into this shortly. I tried this on master and the issue still exists.

[easwars]

I also verified that getting rid of the +1 in https://github.com/grpc/grpc-go/blob/master/rpc_util.go#L744 and https://github.com/grpc/grpc-go/blob/master/rpc_util.go#L750 also gets things to work. But I'm not sure if that is the correct fix at this point. Will investigate more.

[easwars]

The following fix would make it work:

// MaxCallRecvMsgSize returns a CallOption which sets the maximum message size
// in bytes the client can receive.
func MaxCallRecvMsgSize(bytes int) CallOption {
	if bytes == math.MaxInt64 {
		bytes--
	}
	return MaxRecvMsgSizeCallOption{MaxRecvMsgSize: bytes}
}

But, I'm a little concerned that the call options MaxCallRecvMsgSize and MaxCallSendMsgSize accept an int instead of an int32 or int64. With the above fix, someone could still hit the same issue on a 32-bit machine if they passed math.MaxInt32 as their maxCallRecvMsgSize. Should we instead accept a uint32 or unit64? What do you think @dfawley @menghanl ?

[dfawley]

With the above fix, someone could still hit the same issue on a 32-bit machine

We could address that via:

if bytes == math.MaxInt {
	bytes--
}

Note that math.MaxInt is the maximum size for a slice in Go.

Theoretically we should allow MaxInt sized messages, instead of reducing it by 1 in the CallOption - maybe we can be smarter in our code in decompress and not limit the reader to maxReceiveMessageSize+1 which would overflow. Also I'm concerned about this line overflowing in make if the actual message size is 2GB-1, so I think we will need to do more than just subtract 1 here:

https://github.com/grpc/grpc-go/blob/52cea2453436fbb4b962d3cb2da34da7ef6f10c7/rpc_util.go#L743

[easwars]

Hmm ... guess I was blind. I didn't spot the math.MaxInt at all. Will look into the other thing you pointed out.

[Aditya-Sood]

hi @easwars @ginayeh, can I pick this up?

[easwars]

@Aditya-Sood : Sure, go for it. Thank you very much for your contributions and your continued interest in our repo.

Just FYI: I'm going to be out for a few months. @dfawley will be back in a couple of weeks. This issue has been open for a while. So, please talk to @dfawley to make sure that you understand exactly what is required here, so that you don't waste your time going down the wrong path.

[Aditya-Sood]

sure @easwars, hearty congratulations to you and the family! I'll check with @dfawley once he's back happy holidays!

[arvindbr8]

@Aditya-Sood -- ping.. Are you still actively tracking this issue?

[Aditya-Sood]

hi @arvindbr8 apologies for the delay, yes I will work on this I'll go through the context again and ask dfawley for direction before this week ends

[Aditya-Sood]

hi @dfawley, regarding the possible overflow in this statement: https://github.com/grpc/grpc-go/blob/52cea2453436fbb4b962d3cb2da34da7ef6f10c7/rpc_util.go#L743

I think a best-effort solution like this should work:

var bufferSize int
if math.MaxInt-size >= bytes.MinRead {
	bufferSize = size + bytes.MinRead
} else {
	bufferSize = math.MaxInt
}
buf := bytes.NewBuffer(make([]byte, 0, bufferSize))

one query - is the reasoning behind the reduction of message size by 1 in multiple places aimed at accommodating an EOF marker which is added to the messages? if so then shouldn't we just add the check at the point of entry for the message size, to ensure the user input itself is always <= math.MaxInt-1?

[Aditya-Sood]

hi @dfawley @arvindbr8 adding on the last line in the above comment, shall we overwrite an incorrect size to the 4MB default?

func MaxCallRecvMsgSize(bytes int) CallOption {
	if bytes < 0 || bytes == math.MaxInt {
		log.Printf("invalid byte-size (%v) passed to MaxCallRecvMsgSize(), defaulting to 4MB instead", bytes)
		bytes = 4*(1024*1024)
	}
	return MaxRecvMsgSizeCallOption{MaxRecvMsgSize: bytes}
}

[dfawley]

one query - is the reasoning behind the reduction of message size by 1 in multiple places aimed at accommodating an EOF marker which is added to the messages?

The question is: if we limit the reader from the decompressor to the exact size of the limit the user set, then how do we know if the message is bigger than the limit? So instead we limit the reader to limit+1 - if that one last byte is emitted, then the message was over the limit, and we error.

So the -1 is done only to prevent the limit from going over the language's limit for the field (math.MaxInt).

regarding the possible overflow in this statement:

Another way of writing the same thing would be to do the math with uint64s to ensure overflow doesn't happen:

bufferSize := uint64(size) + bytes.MinRead
if bufferSize > math.MaxInt {
	bufferSize = math.MaxInt
}
buf := bytes.NewBuffer(make([]byte, 0, int(bufferSize)))

adding on the last line in the above comment, shall we overwrite an incorrect size to the 4MB default?

I don't like that as much, as we're applying something very different from what the user was intending. How about this?

func decompress() {
	// make buffer
	bytesRead, err := buf.ReadFrom(io.LimitReader(dcReader, maxReceiveMessageSize))
	if bytesRead == maxReceiveMessageSize {
		b := make([]byte,1)
		if n, err := dcReader.Read(b); n > 0 || err != io.EOF {
			// Overflow; return error
		}
	...

[Aditya-Sood]

hi @dfawley, thank you for the inputs, I have created #6999 based on them have also tested it with the helloworld example, the response is no longer empty