[libvirt] [PATCH 1/2] virnetserverservice: Resolve Coverity ARRAY_VS_SINGLETON

[Eric Blake]

On 08/28/2014 01:28 PM, John Ferlan wrote:
> Coverity complained about the following:
> 
> (3) Event ptr_arith:
>    Performing pointer arithmetic on "cur_fd" in expression "cur_fd++".
> 130             return virNetServerServiceNewFD(*cur_fd++,
> 
> It seems the belief is their is pointer arithmetic taking place.  By using
> (*cur_fd)++ we avoid this possible issue

Not just their belief, but the actual C language.

> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/rpc/virnetserverservice.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
> index fea05c3..486f93e 100644
> --- a/src/rpc/virnetserverservice.c
> +++ b/src/rpc/virnetserverservice.c
> @@ -127,7 +127,7 @@ virNetServerServiceNewFDOrUNIX(const char *path,
>           * There's still enough file descriptors.  In this case we'll
>           * use the current one and increment it afterwards.
>           */
> -        return virNetServerServiceNewFD(*cur_fd++,

In this function, cur_fd is int* (four bytes wide).  Suppose cur_fd
starts life as 0x1000, and we start with memory contents of 3 at 0x1000,
and 4 at 0x1004.

C precedence says this is equivalent to *(cur_fd++) - take the pointer
cur_fd, do a pointer post-increment, then dereference the memory at the
location before the increment.  We end up calling
virNetServerServiceNewFD(3) (the contents of cur_fd before the deref),
the memory at 0x1000 remains at 3, and any further uses of cur_fd would
be at the next location 0x1004 - but we just returned so cur_fd is no
longer in scope, so who cares about that change - we could have just
written '*cur_fd' and been done with it.

> +        return virNetServerServiceNewFD((*cur_fd)++,

While this says dereference cur_fd, then post-increment that location.
We still end up calling virNetServerServiceNewFD(3) (the contents of the
memory location before post-increment), but now cur_fd remains at
0x1000, whose contents are now 4.

The caller, daemon/libvirtd.c:daemonSetupNetworking(), calls this
function twice in a row.  Without your patch, it ends up calling
virNetServerServiceNewFD(3) for regular AND read-only sockets.  With
your patch, it does the intended registration of fd 3 and 4.  I hope
that's not a security bug.  Can you trace when this was introduced?

ACK.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140828/f7f82dae/attachment-0001.sig>