[Spice-devel,spice-server,4/8] main-channel: getpeername/getsockname return early if no sockfd

Submitted by Uri Lublin on Oct. 16, 2016, 11:44 a.m.

Details

Message ID 1476618295-26317-5-git-send-email-uril@redhat.com
State Rejected
Headers show
Series "More coverity fixes" ( rev: 2 1 ) in Spice

Not browsing as part of any series.

Commit Message

Uri Lublin Oct. 16, 2016, 11:44 a.m.
I'm not sure that needed as it seems getpeername/getsockname
accept int fd and return -1 for fd=-1

Signed-off-by: Uri Lublin <uril@redhat.com>
---
 server/main-channel.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/main-channel.c b/server/main-channel.c
index bf84694..9ff4dcd 100644
--- a/server/main-channel.c
+++ b/server/main-channel.c
@@ -281,12 +281,24 @@  MainChannelClient *main_channel_link(MainChannel *channel, RedClient *client,
 
 int main_channel_getsockname(MainChannel *main_chan, struct sockaddr *sa, socklen_t *salen)
 {
-    return main_chan ? getsockname(red_channel_get_first_socket(&main_chan->base), sa, salen) : -1;
+    int socketfd;
+
+    if (!main_chan || ((socketfd = red_channel_get_first_socket(&main_chan->base)) < 0)) {
+        return -1;
+    }
+
+    return getsockname(socketfd, sa, salen);
 }
 
 int main_channel_getpeername(MainChannel *main_chan, struct sockaddr *sa, socklen_t *salen)
 {
-    return main_chan ? getpeername(red_channel_get_first_socket(&main_chan->base), sa, salen) : -1;
+    int socketfd;
+
+    if (!main_chan || ((socketfd = red_channel_get_first_socket(&main_chan->base)) < 0)) {
+        return -1;
+    }
+
+    return getpeername(socketfd, sa, salen);
 }
 
 // TODO: ? shouldn't it disonnect all clients? or shutdown all main_channels?

Comments

> 
> I'm not sure that needed as it seems getpeername/getsockname
> accept int fd and return -1 for fd=-1
> 
> Signed-off-by: Uri Lublin <uril@redhat.com>
> ---
>  server/main-channel.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/server/main-channel.c b/server/main-channel.c
> index bf84694..9ff4dcd 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -281,12 +281,24 @@ MainChannelClient *main_channel_link(MainChannel
> *channel, RedClient *client,
>  
>  int main_channel_getsockname(MainChannel *main_chan, struct sockaddr *sa,
>  socklen_t *salen)
>  {
> -    return main_chan ?
> getsockname(red_channel_get_first_socket(&main_chan->base), sa, salen) : -1;
> +    int socketfd;
> +
> +    if (!main_chan || ((socketfd =
> red_channel_get_first_socket(&main_chan->base)) < 0)) {
> +        return -1;
> +    }
> +
> +    return getsockname(socketfd, sa, salen);
>  }
>  
>  int main_channel_getpeername(MainChannel *main_chan, struct sockaddr *sa,
>  socklen_t *salen)
>  {
> -    return main_chan ?
> getpeername(red_channel_get_first_socket(&main_chan->base), sa, salen) : -1;
> +    int socketfd;
> +
> +    if (!main_chan || ((socketfd =
> red_channel_get_first_socket(&main_chan->base)) < 0)) {
> +        return -1;
> +    }
> +
> +    return getpeername(socketfd, sa, salen);
>  }
>  
>  // TODO: ? shouldn't it disonnect all clients? or shutdown all
>  main_channels?

Mumble... I don't know why but it does not look that good.

These functions assume that there are only one client.
They are used only by spice_server_get_sock_info and spice_server_get_peer_info
which are no more used by recent Qemu so mainly they are dead code.
Perhaps spice_server_get_sock_info and spice_server_get_peer_info should
check for the first client and call getpeername/getsockname by themself?
Or just adding a main_channel_get_socket instead of 2 obsolete main_channel_*
functions?

OT: red_channel_get_first_socket is used only for obsolete or wrong code.

OT: main_channel_close is causing a dandling file descriptor.

Frediano
On 10/17/2016 01:38 PM, Frediano Ziglio wrote:
>>
>> I'm not sure that needed as it seems getpeername/getsockname
>> accept int fd and return -1 for fd=-1
>>
>> Signed-off-by: Uri Lublin <uril@redhat.com>
>> ---
>>  server/main-channel.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/server/main-channel.c b/server/main-channel.c
>> index bf84694..9ff4dcd 100644
>> --- a/server/main-channel.c
>> +++ b/server/main-channel.c
>> @@ -281,12 +281,24 @@ MainChannelClient *main_channel_link(MainChannel
>> *channel, RedClient *client,
>>
>>  int main_channel_getsockname(MainChannel *main_chan, struct sockaddr *sa,
>>  socklen_t *salen)
>>  {
>> -    return main_chan ?
>> getsockname(red_channel_get_first_socket(&main_chan->base), sa, salen) : -1;
>> +    int socketfd;
>> +
>> +    if (!main_chan || ((socketfd =
>> red_channel_get_first_socket(&main_chan->base)) < 0)) {
>> +        return -1;
>> +    }
>> +
>> +    return getsockname(socketfd, sa, salen);
>>  }
>>
>>  int main_channel_getpeername(MainChannel *main_chan, struct sockaddr *sa,
>>  socklen_t *salen)
>>  {
>> -    return main_chan ?
>> getpeername(red_channel_get_first_socket(&main_chan->base), sa, salen) : -1;
>> +    int socketfd;
>> +
>> +    if (!main_chan || ((socketfd =
>> red_channel_get_first_socket(&main_chan->base)) < 0)) {
>> +        return -1;
>> +    }
>> +
>> +    return getpeername(socketfd, sa, salen);
>>  }
>>
>>  // TODO: ? shouldn't it disonnect all clients? or shutdown all
>>  main_channels?
>
> Mumble... I don't know why but it does not look that good.
>
> These functions assume that there are only one client.
> They are used only by spice_server_get_sock_info and spice_server_get_peer_info
> which are no more used by recent Qemu so mainly they are dead code.
> Perhaps spice_server_get_sock_info and spice_server_get_peer_info should
> check for the first client and call getpeername/getsockname by themself?
> Or just adding a main_channel_get_socket instead of 2 obsolete main_channel_*
> functions?

Yes, you are correct.
So, should we deprecate those spice_server_get_*_info calls ?

Yes, you are correct.
Currently multi-client is still considered experimental, but we
do have a lot of code to make it almost work.
So, should we deprecate those spice_servet_get_*_info calls ?

>
> OT: red_channel_get_first_socket is used only for obsolete or wrong code.

Due to the assumption there is only one client.

> OT: main_channel_close is causing a dandling file descriptor.

Yes, this function can iterate over all clients and close their
socketfd.

Thanks,
     Uri.
> 
> On 10/17/2016 01:38 PM, Frediano Ziglio wrote:
> >>
> >> I'm not sure that needed as it seems getpeername/getsockname
> >> accept int fd and return -1 for fd=-1
> >>
> >> Signed-off-by: Uri Lublin <uril@redhat.com>
> >> ---
> >>  server/main-channel.c | 16 ++++++++++++++--
> >>  1 file changed, 14 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/server/main-channel.c b/server/main-channel.c
> >> index bf84694..9ff4dcd 100644
> >> --- a/server/main-channel.c
> >> +++ b/server/main-channel.c
> >> @@ -281,12 +281,24 @@ MainChannelClient *main_channel_link(MainChannel
> >> *channel, RedClient *client,
> >>
> >>  int main_channel_getsockname(MainChannel *main_chan, struct sockaddr *sa,
> >>  socklen_t *salen)
> >>  {
> >> -    return main_chan ?
> >> getsockname(red_channel_get_first_socket(&main_chan->base), sa, salen) :
> >> -1;
> >> +    int socketfd;
> >> +
> >> +    if (!main_chan || ((socketfd =
> >> red_channel_get_first_socket(&main_chan->base)) < 0)) {
> >> +        return -1;
> >> +    }
> >> +
> >> +    return getsockname(socketfd, sa, salen);
> >>  }
> >>
> >>  int main_channel_getpeername(MainChannel *main_chan, struct sockaddr *sa,
> >>  socklen_t *salen)
> >>  {
> >> -    return main_chan ?
> >> getpeername(red_channel_get_first_socket(&main_chan->base), sa, salen) :
> >> -1;
> >> +    int socketfd;
> >> +
> >> +    if (!main_chan || ((socketfd =
> >> red_channel_get_first_socket(&main_chan->base)) < 0)) {
> >> +        return -1;
> >> +    }
> >> +
> >> +    return getpeername(socketfd, sa, salen);
> >>  }
> >>
> >>  // TODO: ? shouldn't it disonnect all clients? or shutdown all
> >>  main_channels?
> >
> > Mumble... I don't know why but it does not look that good.
> >
> > These functions assume that there are only one client.
> > They are used only by spice_server_get_sock_info and
> > spice_server_get_peer_info
> > which are no more used by recent Qemu so mainly they are dead code.
> > Perhaps spice_server_get_sock_info and spice_server_get_peer_info should
> > check for the first client and call getpeername/getsockname by themself?
> > Or just adding a main_channel_get_socket instead of 2 obsolete
> > main_channel_*
> > functions?
> 
> Yes, you are correct.
> So, should we deprecate those spice_server_get_*_info calls ?
> 
> Yes, you are correct.
> Currently multi-client is still considered experimental, but we
> do have a lot of code to make it almost work.
> So, should we deprecate those spice_servet_get_*_info calls ?
> 

They are already deprecated in spice-server.h, just retained for
backward compatibility.
Maybe we could just return -1 ??

> >
> > OT: red_channel_get_first_socket is used only for obsolete or wrong code.
> 
> Due to the assumption there is only one client.
> 
> > OT: main_channel_close is causing a dandling file descriptor.
> 
> Yes, this function can iterate over all clients and close their
> socketfd.
> 

Still racing, probably would be better to use shutdown instead
or use dup2 on a /dev/null to atomically replace the file descriptor
or call some other functions.
Currently is called by spice_server_destroy which is never called.
And being spice_server_destroy a final "destructor" call would perhaps
better to just free all resources (memory, sockets and so on).

> Thanks,
>      Uri.
> 
> 

Frediano