msgr: fix large message data content length causing overflow #6809
Conversation
Fix ceph#13985 Signed-off-by: Haomai Wang <haomai@xsky.com> Signed-off-by: Jun Huang <hjwsm1989@gmail.com>
Signed-off-by: Haomai Wang <haomai@xsky.com>
Signed-off-by: Haomai Wang <haomai@xsky.com>
|
size_t isn't a fixed size, so it can be the same value as an int...I think if you really want to handle large messages, it'll need to all be int64_t based. |
|
@gregsfortytwo size_t should be equal to unsigned even in 32bits system, so I guess it should be fine. |
|
Yeah, but the difference between 2GB and 4GB isn't that much, once you start worrying about those sizes. ;) |
|
@gregsfortytwo yes, but for 32bits 4GB is enough because there already exists other limits with 4GB. For 64bits, size_t will be enough compared to before. So I think it make sense? |
|
What other limits do we have that we're going to hit? Are those implementation or protocol issues? I just think if we're going to try and go down this path we should make it pass the big stuff while we're at it. |
|
@gregsfortytwo because 32bits can't have memory larger than 4GB :-) |
|
Whoops, this looks good to me. @athanatos, can you pull into an integration branch? |
| @@ -2461,7 +2461,7 @@ int Pipe::tcp_read(char *buf, int len) | |||
| buf += got; | |||
| //lgeneric_dout(cct, DBL) << "tcp_read got " << got << ", " << len << " left" << dendl; | |||
| } | |||
| return len; | |||
| return 0; | |||
There was a problem hiding this comment.
I am not a ceph specialist, but this seems curious. Why should this function return 0 in case of success? -1 is returned if got is lower zero. So wouldn't it be fine to return got and change return type of this function to ssize_t too?
There was a problem hiding this comment.
yes, it seemed better for the length of read, but you can see the comment of tcp_read in Pipe.h.
/**
* do a blocking read of len bytes from socket
*
* @param buf buffer to read into
* @param len exact number of bytes to read
* @return 0 for success, or -1 on error
*/
int tcp_read(char *buf, int len);
since both are ok, caller doesn't rely on the result code of tcp_read
There was a problem hiding this comment.
Ah, thanks for this explanation. So it was a "bug" because it returned len before?
I often see such stuff in c code. Functions that return success or error but use int to represent this. Why the return type is not bool?
There was a problem hiding this comment.
Hmm, it may not be a big problem, just left "int" is fine I think
msgr: fix large message data content length causing overflow Reviewed-by: Greg Farnum <gfarnum@redhat.com>
ghost
changed the title
Fix #13985