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