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@xxxxxxxxxx>
>> ---
>> 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
>From 438ee86a8bd2f060ce87601d60636f92a88932d1 Mon Sep 17 00:00:00 2001
From: John Ferlan <jferlan@xxxxxxxxxx>
Date: Thu, 28 Aug 2014 09:03:47 -0400
Subject: [PATCH 1/2] virnetserverservice: Resolve Coverity ARRAY_VS_SINGLETON
Coverity complained about the following:
(3) Event ptr_arith:
Performing pointer arithmetic on "cur_fd" in expression "cur_fd++".
130 return virNetServerServiceNewFD(*cur_fd++,
The complain is that pointer arithmetic taking place instead of the
expected auto increment of the variable... Rework the code to be a bit
more careful and void auto increment arithmetic on a pointer.
Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---
src/rpc/virnetserverservice.c | 35 +++++++++++++++++++++--------------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
index fea05c3..d6b4024 100644
--- a/src/rpc/virnetserverservice.c
+++ b/src/rpc/virnetserverservice.c
@@ -106,36 +106,43 @@ virNetServerServiceNewFDOrUNIX(const char *path,
unsigned int nfds,
unsigned int *cur_fd)
{
+ virNetServerServicePtr ret;
if (*cur_fd - STDERR_FILENO > nfds) {
/*
* There are no more file descriptors to use, so we have to
* fallback to UNIX socket.
*/
- return virNetServerServiceNewUNIX(path,
- mask,
- grp,
- auth,
+ ret = virNetServerServiceNewUNIX(path,
+ mask,
+ grp,
+ auth,
#if WITH_GNUTLS
- tls,
+ tls,
#endif
- readonly,
- max_queued_clients,
- nrequests_client_max);
+ readonly,
+ max_queued_clients,
+ nrequests_client_max);
} else {
/*
* There's still enough file descriptors. In this case we'll
* use the current one and increment it afterwards.
*/
- return virNetServerServiceNewFD(*cur_fd++,
- auth,
+ ret = virNetServerServiceNewFD(*cur_fd,
+ auth,
#if WITH_GNUTLS
- tls,
+ tls,
#endif
- readonly,
- max_queued_clients,
- nrequests_client_max);
+ readonly,
+ max_queued_clients,
+ nrequests_client_max);
+ /* Be very careful - don't use autoincrement
+ * as that becomes pointer arithmetic which is
+ * not what is desired here!
+ */
+ *cur_fd = *cur_fd + 1;
}
+ return ret;
}
--
1.9.3
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list