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 makeLimitReader.Nbe negative value and return nothing

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

Thanks@lysufor 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 theClientConnwork:

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 whenmath.MaxInt64is used as themaxCallRecvMsgSizethat 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.

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

The following fix would make it work:

// MaxCallRecvMsgSize returns a CallOption which sets the maximum message size
// in bytes the client can receive.
funcMaxCallRecvMsgSize(bytesint)CallOption{
ifbytes==math.MaxInt64{
bytes--
}
returnMaxRecvMsgSizeCallOption{MaxRecvMsgSize:bytes}
}

But, I'm a little concerned that the call optionsMaxCallRecvMsgSizeandMaxCallSendMsgSizeaccept anintinstead of anint32orint64.With the above fix, someone could still hit the same issue on a 32-bit machine if they passedmath.MaxInt32as theirmaxCallRecvMsgSize.Should we instead accept auint32orunit64?What do you think@dfawley@menghanl?

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 thatmath.MaxIntis the maximum size for a slice in Go.

Theoretically we should allowMaxIntsized messages, instead of reducing it by 1 in theCallOption- maybe we can be smarter in our code indecompressand not limit the reader tomaxReceiveMessageSize+1which would overflow. Also I'm concerned about this line overflowing inmakeif the actual message size is 2GB-1, so I think we will need to do more than just subtract 1 here:

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

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

@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.@dfawleywill be back in a couple of weeks. This issue has been open for a while. So, please talk to@dfawleyto make sure that you understand exactly what is required here, so that you don't waste your time going down the wrong path.

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

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

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

hi@dfawley,
regarding the possible overflow in this statement:

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?

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}
}

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 tolimit+1- if that one last byte is emitted, then the message was over the limit, and we error.

So the-1is 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
ifbufferSize>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?

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

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

@Aditya-Soodare you still working on this issue? If not, I'll pick it up.

yes arjan

@purnesh42Hplease assign this issue to me

@arjan-balkindly assign the issue to Vivek, I keep getting side-tracked with other work so will not be able to commit as of now

@infovivek2020there's an incomplete PR for this issue (#6999) which you can reference

@purnesh42Hraised PR#7468my question in PR itself can suggest what i need do

Thedecompressfunction was optimized to use a pool of small buffers (mem.BufferSlice) instead of one large buffer. However, it still doio.Copy(mem.NewWriter(&out, pool), io.LimitReader(dcReader, int64(maxReceiveMessageSize) + 1))to copy data from the decompressed stream (dcReader) to the output buffer (out).

The mentioned bug, however, still exist: when the maximum receive message size (maxReceiveMessageSize) is very large (greater than or equal tomath.MaxInt),io.LimitReadercreates a reader with a negative size, causing theiopackage to fail validation and return an empty response. This happens even if the actual number of bytes read is less thanmath.MaxInt.

To fix this, we can use thecheckReceiveMessageOverflowmethod (provided by@Aditya-Sood) to validate the message size after copying the data. Here are two possible approaches:

Approach 1

if maxReceiveMessageSize >= math.MaxInt {
_, err = io.Copy(mem.NewWriter(&out, pool), io.LimitReader(dcReader, int64(math.MaxInt)))
if err == nil {
err = checkReceiveMessageOverflow(int64(out.Len()), int64(maxReceiveMessageSize), dcReader)
if err!= nil {
return nil, out.Len() + 1, err
}
}
} else {
_, err = io.Copy(mem.NewWriter(&out, pool), io.LimitReader(dcReader, int64(maxReceiveMessageSize)+1))
}

Approach 2

_, err = io.Copy(mem.NewWriter(&out, pool), io.LimitReader(dcReader, int64(maxReceiveMessageSize)))
if err!= nil {
out.Free()
return nil, 0, err
}
if err = checkReceiveMessageOverflow(int64(out.Len()), int64(maxReceiveMessageSize), dcReader); err!= nil {
return nil, out.Len() + 1, err
}

@dfawley@easwarslet me know what you think?

@infovivek2020fyi