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

[Daniel P. Berrange]

On Tue, Oct 15, 2013 at 10:46:59AM +0800, Osier Yang wrote:
> 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.

I don't really find this new version more readable. If you removed the
" + VIR_NET_MESSAGE_LEN_MAX" from both the 'newlen' initialization and
the 'if()' check, then it might be more readable....


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

...and added  "+ VIR_NET_MESSAGE_LEN_MAX" here.

> >
> >>          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)"),


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|