Re: [PATCH] network: Resolve Coverity FORWARD_NULL

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

 



On 16.03.2015 14:16, John Ferlan wrote:
> The following is a long winded way to say this patch is avoiding a
> false positive.
> 
> Coverity complains that calling networkPlugBandwidth() could eventually
> end up with a NULL dereference on iface->bandwidth because in the
> networkAllocateActualDevice there's a check of 'iface->bandwidth'
> before deciding to try to use the 'portgroup' if it exists or to not
> perferm the virNetDevBandwidthCopy if 'bandwidth' is not NULL.
> 
> Later in networkPlugBandwidth the 'iface->bandwidth' is sourced from
> virDomainNetGetActualBandwidth - which would be either iface->bandwidth
> or (preferably) iface->data.network.actual->bandwidth which would have
> been filled in from either 'iface->bandwidth' or 'portgroup->bandwidth'
> back in networkAllocateActualDevice
> 
> There *is* a check in networkCheckBandwidth for the result of the
> virDomainNetGetActualBandwidth being NULL and a return 1 based on
> that which would cause networkPlugBandwidth to exit properly and thus
> never hit the condition that Coverity complains about.
> 
> However, since Coverity checks all paths - it somehow believes that
> a return of 0 by networkCheckBandwidth in this condition would end
> up causing the possible NULL dereference. The "fix" to silence Coverity
> is to not have networkCheckBandwidth also call virDomainNetGetActualBandwidth
> in order to get the ifaceBand, but rather have it accept it as an argument
> which causes Coverity to "see" that it's the exit condition of 1 that won't
> have the possible NULL dereference.  Since we're passing that, I added the
> passing of iface->mac rather than passing iface as well. This just hopefully
> makes sure someone doesn't undo this in the future...
> 
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
>  src/network/bridge_driver.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index a007388..d885aa9 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -3820,7 +3820,7 @@ networkAllocateActualDevice(virDomainDefPtr dom,
>          (netdef->forward.type == VIR_NETWORK_FORWARD_NAT) ||
>          (netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE)) {
>          /* for these forward types, the actual net type really *is*
> -         *NETWORK; we just keep the info from the portgroup in
> +         * NETWORK; we just keep the info from the portgroup in
>           * iface->data.network.actual
>           */
>          iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_NETWORK;
> @@ -4593,17 +4593,17 @@ networkGetNetworkAddress(const char *netname, char **netaddr)
>   */
>  static int
>  networkCheckBandwidth(virNetworkObjPtr net,
> -                      virDomainNetDefPtr iface,
> +                      virNetDevBandwidthPtr ifaceBand,
> +                      virMacAddr ifaceMac,
>                        unsigned long long *new_rate)

Unfortunately not to be seen in this little context, but there's a
comment just before these lines describing function arguments. You need
to update it too.

>  {
>      int ret = -1;
>      virNetDevBandwidthPtr netBand = net->def->bandwidth;
> -    virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
>      unsigned long long tmp_floor_sum = net->floor_sum;
>      unsigned long long tmp_new_rate = 0;
>      char ifmac[VIR_MAC_STRING_BUFLEN];
>  
> -    virMacAddrFormat(&iface->mac, ifmac);
> +    virMacAddrFormat(&ifaceMac, ifmac);
>  
>      if (ifaceBand && ifaceBand->in && ifaceBand->in->floor &&
>          !(netBand && netBand->in)) {
> @@ -4689,7 +4689,8 @@ networkPlugBandwidth(virNetworkObjPtr net,
>      char ifmac[VIR_MAC_STRING_BUFLEN];
>      virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
>  
> -    if ((plug_ret = networkCheckBandwidth(net, iface, &new_rate)) < 0) {
> +    if ((plug_ret = networkCheckBandwidth(net, ifaceBand,
> +                                          iface->mac, &new_rate)) < 0) {
>          /* helper reported error */
>          goto cleanup;
>      }
> 

Sad we must do this just to silent Coverity. ACK if you update te
networkCheckBandwidth comment.

Michal

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