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

John Ferlan jferlan at redhat.com
Thu Aug 28 22:04:33 UTC 2014



On 08/28/2014 04:57 PM, Eric Blake wrote:
> 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.
> 

So yes, I was being a little facetious too. I cut off part of an
original "comment" regarding auto increment pointer arithmetic... It is
almost always prime for "issues" - one of those language constructs
useful for so many things, but oh so dangerous.

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

For the benefit of others ;-) as Eric and I have been IRC'g over this...
This was introduced in this release cycle (commit id '9805256d')...  So
not in the wild yet (fairly recent commit of 8/22)

Although I know you ACK'd what I have... How about the attached instead?
It avoids the auto incremented pointer arithmetic (but of course still
has an assumption regarding fd's being sequential).  It looks like more
changed only because of the formatting to use "ret = " instead of
"return" - just needed one more character in my variable name to avoid
lines of single space adjustment.

John

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-virnetserverservice-Resolve-Coverity-ARRAY_VS_SINGLE.patch
Type: text/x-patch
Size: 3298 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140828/6323b3b6/attachment-0001.bin>


More information about the libvir-list mailing list