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

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.

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.

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 ?

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:

Hmm ... guess I was blind. I didn't spot the math.MaxInt at 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. @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.

sure @easwars, hearty congratulations to you and the family!
I'll check with @dfawley once 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 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
		}
	...

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

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

yes arjan

@purnesh42H please assign this issue to me

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

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

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

The decompress function was optimized to use a pool of small buffers (mem.BufferSlice) instead of one large buffer. However, it still do io.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 to math.MaxInt), io.LimitReader creates a reader with a negative size, causing the io package to fail validation and return an empty response. This happens even if the actual number of bytes read is less than math.MaxInt.

To fix this, we can use the checkReceiveMessageOverflow method (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 @easwars let me know what you think?

@infovivek2020 fyi