From e5bec5d00819d6fe71a66ee022d1e9d9acd4fe5c Mon Sep 17 00:00:00 2001 From: Omer Tuchfeld Date: Fri, 8 Jul 2022 08:05:57 +0200 Subject: [PATCH] Fix `networkRange` race condition and global state corruption (#945) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While the `networkRange` function calculates `lastIP`, it uses `net.IPv4zero` or `net.IPv6zero` as a starting value from which it builds the final last IP. The issue is that these are global variables defined in the standard library, and modifying them affects all future users of those variables across the program. This leads to a not-so-rare race condition when creating multiple libvirt networks. These networks are being created concurrently and they both race modifying that global variable. Their `lastIP` DHCP configuration gets mixed up as result. The manner in which the configuration gets mixed up is unpredictable, but it could lead to a corrupt configuration that cannot be applied. The solution is to copy the zero IP addresses to a new variable rather than use them directly. Instead for simplicity I just instantiate an empty slice with the same length as the net IP, which would be filled with zeroes anyway. This commit also solves another unrelated bug in `getNetworkIPConfig` where the `^` operator was used as if it's a power operator, while in reality it's a bitwise-xor that leads to slightly incorrect results. This makes the validation only slightly wrong and is overly strict (e.g. it will not allow you to create `/28` IPv4 network even though such network has far more available addresses than the condition blocks) A similar race condition can be simply reproduced and visualized with this short go code: ```go package main import ( "fmt" "net" "sync" ) func getNetMaskWithMax16Bits(m net.IPMask) net.IPMask { ones, bits := m.Size() if bits-ones > 16 { if bits == 128 { return net.CIDRMask(128-16, 128) } return net.CIDRMask(32-16, 32) } return m } func networkRange(network *net.IPNet) (net.IP, net.IP) { netIP := network.IP.To4() lastIP := net.IPv4zero.To4() if netIP == nil { netIP = network.IP.To16() lastIP = net.IPv6zero.To16() } firstIP := netIP.Mask(network.Mask) intMask := getNetMaskWithMax16Bits(network.Mask) for i := 0; i < len(lastIP); i++ { lastIP[i] = netIP[i] | ^intMask[i] } return firstIP, lastIP } func update(wg *sync.WaitGroup, cidr string, id int) { address := cidr _, ipNet, _ := net.ParseCIDR(address) start, end := networkRange(ipNet) start[len(start)-1]++ start[len(start)-1]++ end[len(end)-1]-- fmt.Printf("Start %d: %s\n", id, start.String()) fmt.Printf("End %d: %s\n", id, end.String()) wg.Done() } func main() { var wg sync.WaitGroup wg.Add(2) go update(&wg, "192.168.145.0/24", 0) go update(&wg, "192.168.127.0/24", 1) wg.Wait() } ``` Then run: ```bash watch -n0.1 -d go run main.go ``` To see it happen for short moments. Alternatively, create this `main.tf` file: ```terraform terraform { required_providers { libvirt = { source = "dmacvicar/libvirt" version = "0.6.14" } } } provider "libvirt" { uri = "qemu:///system" } resource "libvirt_network" "net" { name = "omer-net" mode = "nat" addresses = ["192.168.127.0/24"] autostart = true } resource "libvirt_network" "secondary_net" { name = "omer-second-net" mode = "nat" addresses = ["192.168.145.0/24"] autostart = true } ``` And run: ```bash while true; do if ! (terraform apply -auto-approve && terraform destroy -auto-approve); then break fi done ``` Until the bug reproduces with the following error: ``` │ Error: error defining libvirt network: internal error: range 192.168.127.2 - 192.168.145.254 is not entirely within network 192.168.127.1/24 - │ omer-net │ │ │ │ │ │ │ │ │ │ │ │ with libvirt_network.net, │ on main.tf line 14, in resource "libvirt_network" "net": │ 14: resource "libvirt_network" "net" { ``` --- libvirt/network.go | 2 +- libvirt/utils_net.go | 28 ++++++++++++++++------------ 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/libvirt/network.go b/libvirt/network.go index a87ff94d1..7b945f70f 100644 --- a/libvirt/network.go +++ b/libvirt/network.go @@ -97,7 +97,7 @@ func getNetworkIPConfig(address string) (*libvirtxml.NetworkIP, *libvirtxml.Netw if bits == (net.IPv6len * 8) { family = "ipv6" } - ipsRange := 2 ^ bits - 2 ^ ones + ipsRange := (1 << bits) - (1 << ones) if ipsRange < 4 { return nil, nil, fmt.Errorf("netmask seems to be too strict: only %d IPs available (%s)", ipsRange-3, family) } diff --git a/libvirt/utils_net.go b/libvirt/utils_net.go index 83f09a862..46f6b9357 100644 --- a/libvirt/utils_net.go +++ b/libvirt/utils_net.go @@ -86,22 +86,26 @@ func getNetMaskWithMax16Bits(m net.IPMask) net.IPMask { return m } -// networkRange calculates the first and last IP addresses in an IPNet -func networkRange(network *net.IPNet) (net.IP, net.IP) { - netIP := network.IP.To4() - lastIP := net.IPv4zero.To4() - if netIP == nil { - netIP = network.IP.To16() - lastIP = net.IPv6zero.To16() - } - firstIP := netIP.Mask(network.Mask) +func getLastIP(network *net.IPNet, netIP net.IP) net.IP { + lastIP := make(net.IP, len(netIP)) + // intermediate network mask with max 16 bits for hosts // We need a mask with max 16 bits since libvirt only supports 65535) IP's per subnet // 2^16 = 65536 (minus broadcast and .1) intMask := getNetMaskWithMax16Bits(network.Mask) + for i, netIPByte := range netIP { + lastIP[i] = netIPByte | ^intMask[i] + } - for i := 0; i < len(lastIP); i++ { - lastIP[i] = netIP[i] | ^intMask[i] + return lastIP +} + +// networkRange calculates the first and last IP addresses in an IPNet +func networkRange(network *net.IPNet) (firstIP net.IP, lastIP net.IP) { + netIP := network.IP.To4() + if netIP == nil { + netIP = network.IP.To16() } - return firstIP, lastIP + + return netIP.Mask(network.Mask), getLastIP(network, netIP) }