Re: [libvirt] [PATCH] rpc: Correct the wrong payload size checking

On 14/10/13 21:26, Daniel P. Berrange wrote: >On Mon, Oct 14, 2013 at 03:22:27PM +0800, Osier Yang wrote: >><...> >>/* Size of message length field. Not counted in VIR_NET_MESSAGE_MAX >> * and VIR_NET_MESSAGE_INITIAL. >> */ >>const VIR_NET_MESSAGE_LEN_MAX = 4; >></...> >> >>However, msg->bufferLength includes the length word. The wrong checking >>was introduced by commit e914dcfd. >> >>* src/rpc/virnetmessage.c: >> - Correct the checking in virNetMessageEncodePayloadRaw >> - Use a new variable to track the new payload length in >> virNetMessageEncodePayloadRaw >>--- >> src/rpc/virnetmessage.c | 19 +++++++++++++------ >> 1 file changed, 13 insertions(+), 6 deletions(-) >> >>diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c >>index 8f4e4bc..4c60424 100644 >>--- a/src/rpc/virnetmessage.c >>+++ b/src/rpc/virnetmessage.c >>@@ -346,15 +346,17 @@ int virNetMessageEncodePayload(virNetMessagePtr msg, >> /* Try to encode the payload. If the buffer is too small increase it. */ >> while (!(*filter)(&xdr, data)) { >>- if ((msg->bufferLength - VIR_NET_MESSAGE_LEN_MAX) * 4 > VIR_NET_MESSAGE_MAX) { >>+ unsigned int newlen = (msg->bufferLength - VIR_NET_MESSAGE_LEN_MAX) * 4 + >>+ VIR_NET_MESSAGE_LEN_MAX; >>+ >>+ if (newlen > VIR_NET_MESSAGE_MAX + VIR_NET_MESSAGE_LEN_MAX) { >You've not actually changed the logic here at all - you've just >added VIR_NET_MESSAGE_LEN_MAX to both sides of the '>'. So this >change is a no-op. Please drop it. Yes, the logic is not changed, but I wanted to calculate the new buffer length once instead of twice, and it's more readable for the code. So I'd like keep it.

> > >> virReportError(VIR_ERR_RPC, "%s", _("Unable to encode message payload")); >> goto error; >> } >> xdr_destroy(&xdr); >>- msg->bufferLength = (msg->bufferLength - VIR_NET_MESSAGE_LEN_MAX) * 4 + >>- VIR_NET_MESSAGE_LEN_MAX; >>+ msg->bufferLength = newlen; >Again, nothing changed. Drop this.

> >> if (VIR_REALLOC_N(msg->buffer, msg->bufferLength) < 0) >> goto error; >>@@ -426,10 +428,15 @@ int virNetMessageEncodePayloadRaw(virNetMessagePtr msg, >> /* If the message buffer is too small for the payload increase it accordingly. */ >> if ((msg->bufferLength - msg->bufferOffset) < len) { >>- if ((msg->bufferOffset + len) > VIR_NET_MESSAGE_MAX) { >>+ if ((msg->bufferOffset + len) > >>+ VIR_NET_MESSAGE_MAX + VIR_NET_MESSAGE_LEN_MAX) { >Bracket both sides of the '>' for clarity, not just the left-hand-side. > So except this, Can I get an ACK?

diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c index 4c60424..79a50f6 100644 --- a/src/rpc/virnetmessage.c +++ b/src/rpc/virnetmessage.c @@ -429,7 +429,7 @@ int virNetMessageEncodePayloadRaw(virNetMessagePtr msg, /* If the message buffer is too small for the payload increase it accordingly. */ if ((msg->bufferLength - msg->bufferOffset) < len) { if ((msg->bufferOffset + len) > - VIR_NET_MESSAGE_MAX + VIR_NET_MESSAGE_LEN_MAX) { + (VIR_NET_MESSAGE_MAX + VIR_NET_MESSAGE_LEN_MAX)) { virReportError(VIR_ERR_RPC, _("Stream data too long to send " "(%zu bytes needed, %zu bytes available)"),