Re: [PATCH] Fix possible invalid read in adminClientGetInfo

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 29/06/16 07:10, Ján Tomko wrote:
> virNetServerClientGetInfo returns the client's remote address
> as a string, which is a part of the client object.
> 
> Use VIR_STRDUP to make a copy which can be freely accessed
> even after the virNetServerClient object is unlocked.

Although you're right, it was a bit difficult to see what kind of bug
are referring to with this fix, but now I see it...however, would you
mind adding the reproducer to the commit message (for future purposes)? :)

> ---
>  daemon/admin_server.c        | 3 ++-
>  src/rpc/virnetserverclient.c | 8 ++++++--
>  src/rpc/virnetserverclient.h | 2 +-
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/daemon/admin_server.c b/daemon/admin_server.c
> index cb9079c..9f24f68 100644
> --- a/daemon/admin_server.c
> +++ b/daemon/admin_server.c
> @@ -221,7 +221,7 @@ adminClientGetInfo(virNetServerClientPtr client,
>      int ret = -1;
>      int maxparams = 0;
>      bool readonly;
> -    const char *sock_addr = NULL;
> +    char *sock_addr = NULL;
>      const char *attr = NULL;
>      virTypedParameterPtr tmpparams = NULL;
>      virIdentityPtr identity = NULL;
> @@ -300,6 +300,7 @@ adminClientGetInfo(virNetServerClientPtr client,
>  
>   cleanup:
>      virObjectUnref(identity);
> +    VIR_FREE(sock_addr);
>      return ret;
>  }
>  
> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> index 39fe860..81da82c 100644
> --- a/src/rpc/virnetserverclient.c
> +++ b/src/rpc/virnetserverclient.c
> @@ -1606,20 +1606,24 @@ virNetServerClientGetTransport(virNetServerClientPtr client)
>  
>  int
>  virNetServerClientGetInfo(virNetServerClientPtr client,
> -                          bool *readonly, const char **sock_addr,
> +                          bool *readonly, char **sock_addr,
>                            virIdentityPtr *identity)
>  {
>      int ret = -1;
> +    const char *addr;
>  
>      virObjectLock(client);
>      *readonly = client->readonly;
>  
> -    if (!(*sock_addr = virNetServerClientRemoteAddrStringURI(client))) {
> +    if (!(addr = virNetServerClientRemoteAddrStringURI(client))) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("No network socket associated with client"));
>          goto cleanup;
>      }
>  
> +    if (VIR_STRDUP(*sock_addr, addr) < 0)
> +        goto cleanup;
> +
>      if (!client->identity) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("No identity information available for client"));
> diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
> index c243a68..a53cc00 100644
> --- a/src/rpc/virnetserverclient.h
> +++ b/src/rpc/virnetserverclient.h
> @@ -149,7 +149,7 @@ bool virNetServerClientNeedAuth(virNetServerClientPtr client);
>  
>  int virNetServerClientGetTransport(virNetServerClientPtr client);
>  int virNetServerClientGetInfo(virNetServerClientPtr client,
> -                              bool *readonly, const char **sock_addr,
> +                              bool *readonly, char **sock_addr,
>                                virIdentityPtr *identity);
>  
>  #endif /* __VIR_NET_SERVER_CLIENT_H__ */
> 

Good catch, ACK.

Erik

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]