From 09d56f54a7698e5768b25ee43a47b165e8aebbf9 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 1 Sep 2020 13:24:58 -0700 Subject: [PATCH] wgengine/router: fix Windows route sorting that caused de-dup to not work (#727) Updates #725 Signed-off-by: Brad Fitzpatrick --- wgengine/router/ifconfig_windows.go | 32 +++++--- wgengine/router/ifconfig_windows_test.go | 97 ++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 9 deletions(-) create mode 100644 wgengine/router/ifconfig_windows_test.go diff --git a/wgengine/router/ifconfig_windows.go b/wgengine/router/ifconfig_windows.go index 78388ccc0..784c6a476 100644 --- a/wgengine/router/ifconfig_windows.go +++ b/wgengine/router/ifconfig_windows.go @@ -315,15 +315,7 @@ func configureInterface(cfg *Config, tun *tun.NativeTun) error { return err } - sort.Slice(routes, func(i, j int) bool { - return (bytes.Compare(routes[i].Destination.IP, routes[j].Destination.IP) == -1 || - // Narrower masks first - bytes.Compare(routes[i].Destination.Mask, routes[j].Destination.Mask) == 1 || - // No nexthop before non-empty nexthop - bytes.Compare(routes[i].NextHop, routes[j].NextHop) == -1 || - // Lower metrics first - routes[i].Metric < routes[j].Metric) - }) + sort.Slice(routes, func(i, j int) bool { return routeLess(&routes[i], &routes[j]) }) deduplicatedRoutes := []*winipcfg.RouteData{} for i := 0; i < len(routes); i++ { @@ -387,3 +379,25 @@ func configureInterface(cfg *Config, tun *tun.NativeTun) error { return errAcc } + +// routeLess reports whether ri should sort before rj. +// The actual sort order doesn't appear to matter. The caller just +// wants them sorted to be able to de-dup. +func routeLess(ri, rj *winipcfg.RouteData) bool { + if v := bytes.Compare(ri.Destination.IP, rj.Destination.IP); v != 0 { + return v == -1 + } + if v := bytes.Compare(ri.Destination.Mask, rj.Destination.Mask); v != 0 { + // Narrower masks first + return v == 1 + } + if ri.Metric != rj.Metric { + // Lower metrics first + return ri.Metric < rj.Metric + } + if v := bytes.Compare(ri.NextHop, rj.NextHop); v != 0 { + // No nexthop before non-empty nexthop. + return v == -1 + } + return false +} diff --git a/wgengine/router/ifconfig_windows_test.go b/wgengine/router/ifconfig_windows_test.go new file mode 100644 index 000000000..d8e0852e8 --- /dev/null +++ b/wgengine/router/ifconfig_windows_test.go @@ -0,0 +1,97 @@ +// Copyright (c) 2020 Tailscale Inc & AUTHORS All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package router + +import ( + "math/rand" + "net" + "testing" + + winipcfg "github.com/tailscale/winipcfg-go" + "inet.af/netaddr" +) + +func randIP() net.IP { + b := byte(rand.Intn(3)) + return net.IP{b, b, b, b} +} + +func randRouteData() *winipcfg.RouteData { + return &winipcfg.RouteData{ + Destination: net.IPNet{ + IP: randIP(), + Mask: net.CIDRMask(rand.Intn(3)+1, 32), + }, + NextHop: randIP(), + Metric: uint32(rand.Intn(3)), + } +} + +func TestRouteLess(t *testing.T) { + type D = winipcfg.RouteData + ipnet := func(s string) net.IPNet { + ipp, err := netaddr.ParseIPPrefix(s) + if err != nil { + t.Fatalf("error parsing test data %q: %v", s, err) + } + return *ipp.IPNet() + } + + tests := []struct { + ri, rj *winipcfg.RouteData + want bool + }{ + { + ri: &D{Metric: 1}, + rj: &D{Metric: 2}, + want: true, + }, + { + ri: &D{Destination: ipnet("1.1.0.0/16"), Metric: 2}, + rj: &D{Destination: ipnet("2.2.0.0/16"), Metric: 1}, + want: true, + }, + { + ri: &D{Destination: ipnet("1.1.0.0/16"), Metric: 1}, + rj: &D{Destination: ipnet("2.2.0.0/16"), Metric: 1}, + want: true, + }, + { + ri: &D{Destination: ipnet("1.1.0.0/32"), Metric: 2}, + rj: &D{Destination: ipnet("1.1.0.0/16"), Metric: 1}, + want: true, + }, + { + ri: &D{Destination: ipnet("1.1.0.0/32"), Metric: 1}, + rj: &D{Destination: ipnet("1.1.0.0/16"), Metric: 1}, + want: true, + }, + { + ri: &D{Destination: ipnet("1.1.0.0/16"), Metric: 1, NextHop: net.ParseIP("3.3.3.3")}, + rj: &D{Destination: ipnet("1.1.0.0/16"), Metric: 1, NextHop: net.ParseIP("4.4.4.4")}, + want: true, + }, + } + for i, tt := range tests { + got := routeLess(tt.ri, tt.rj) + if got != tt.want { + t.Errorf("%v. less = %v; want %v", i, got, tt.want) + } + back := routeLess(tt.rj, tt.ri) + if back && got { + t.Errorf("%v. less both ways", i) + } + } +} + +func TestRouteLessConsistent(t *testing.T) { + for i := 0; i < 10000; i++ { + ri := randRouteData() + rj := randRouteData() + if routeLess(ri, rj) && routeLess(rj, ri) { + t.Fatalf("both compare less to each other:\n\t%#v\nand\n\t%#v", ri, rj) + } + } +}