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

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(a)redhat.com&gt; > --- > 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.