net/netip: fix invalid representation of Prefix

For a given Addr, ensure there is exactly one invalid representation.
This allows invalid representations to be safely comparable.
To ensure that the zero value of Prefix is invalid,
we modify the encoding of bits to simply be the bit count plus one.

Since Addr is immutable, we check in the PrefixFrom constructor that
the provided Addr is valid and only store a non-zero bits length if so.
IsValid is simplified to just checking whether bitsPlusOne is non-zero.

Fixes #54525

Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
Reviewed-on: https://go-review.googlesource.com/c/go/+/425035
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>

diff --git a/src/net/netip/netip.go b/src/net/netip/netip.go
index ec9266d583cf7f9121c484073dca73cb2d88d5f7..aa700f46a7bf8ba862517e8618741a2d2180e8ef 100644 (file)
--- a/src/net/netip/netip.go
+++ b/src/net/netip/netip.go
@@ -1225,20 +1225,11 @@ func (p *AddrPort) UnmarshalBinary(b []byte) error {
 type Prefix struct {
        ip Addr
 
-       // bits is logically a uint8 (storing [0,128]) but also
-       // encodes an "invalid" bit, currently represented by the
-       // invalidPrefixBits sentinel value. It could be packed into
-       // the uint8 more with more complicated expressions in the
-       // accessors, but the extra byte (in padding anyway) doesn't
-       // hurt and simplifies code below.
-       bits int16
+       // bitsPlusOne stores the prefix bit length plus one.
+       // A Prefix is valid if and only if bitsPlusOne is non-zero.
+       bitsPlusOne uint8
 }
 
-// invalidPrefixBits is the Prefix.bits value used when PrefixFrom is
-// outside the range of a uint8. It's returned as the int -1 in the
-// public API.
-const invalidPrefixBits = -1
-
 // PrefixFrom returns a Prefix with the provided IP address and bit
 // prefix length.
 //
@@ -1248,13 +1239,13 @@ const invalidPrefixBits = -1
 // If bits is less than zero or greater than ip.BitLen, Prefix.Bits
 // will return an invalid value -1.
 func PrefixFrom(ip Addr, bits int) Prefix {
-       if bits < 0 || bits > ip.BitLen() {
-               bits = invalidPrefixBits
+       var bitsPlusOne uint8
+       if !ip.isZero() && bits >= 0 && bits <= ip.BitLen() {
+               bitsPlusOne = uint8(bits) + 1
        }
-       b16 := int16(bits)
        return Prefix{
-               ip:   ip.withoutZone(),
-               bits: b16,
+               ip:          ip.withoutZone(),
+               bitsPlusOne: bitsPlusOne,
        }
 }
 
@@ -1264,17 +1255,17 @@ func (p Prefix) Addr() Addr { return p.ip }
 // Bits returns p's prefix length.
 //
 // It reports -1 if invalid.
-func (p Prefix) Bits() int { return int(p.bits) }
+func (p Prefix) Bits() int { return int(p.bitsPlusOne) - 1 }
 
 // IsValid reports whether p.Bits() has a valid range for p.Addr().
 // If p.Addr() is the zero Addr, IsValid returns false.
 // Note that if p is the zero Prefix, then p.IsValid() == false.
-func (p Prefix) IsValid() bool { return !p.ip.isZero() && p.bits >= 0 && int(p.bits) <= p.ip.BitLen() }
+func (p Prefix) IsValid() bool { return p.bitsPlusOne > 0 }
 
 func (p Prefix) isZero() bool { return p == Prefix{} }
 
 // IsSingleIP reports whether p contains exactly one IP.
-func (p Prefix) IsSingleIP() bool { return p.bits != 0 && int(p.bits) == p.ip.BitLen() }
+func (p Prefix) IsSingleIP() bool { return p.IsValid() && p.Bits() == p.ip.BitLen() }
 
 // ParsePrefix parses s as an IP address prefix.
 // The string can be in the form "192.168.1.0/24" or "2001:db8::/32",
@@ -1327,10 +1318,8 @@ func MustParsePrefix(s string) Prefix {
 //
 // If p is zero or otherwise invalid, Masked returns the zero Prefix.
 func (p Prefix) Masked() Prefix {
-       if m, err := p.ip.Prefix(int(p.bits)); err == nil {
-               return m
-       }
-       return Prefix{}
+       m, _ := p.ip.Prefix(p.Bits())
+       return m
 }
 
 // Contains reports whether the network p includes ip.
@@ -1356,12 +1345,12 @@ func (p Prefix) Contains(ip Addr) bool {
                // the compiler doesn't know that, so mask with 63 to help it.
                // Now truncate to 32 bits, because this is IPv4.
                // If all the bits we care about are equal, the result will be zero.
-               return uint32((ip.addr.lo^p.ip.addr.lo)>>((32-p.bits)&63)) == 0
+               return uint32((ip.addr.lo^p.ip.addr.lo)>>((32-p.Bits())&63)) == 0
        } else {
                // xor the IP addresses together.
                // Mask away the bits we don't care about.
                // If all the bits we care about are equal, the result will be zero.
-               return ip.addr.xor(p.ip.addr).and(mask6(int(p.bits))).isZero()
+               return ip.addr.xor(p.ip.addr).and(mask6(p.Bits())).isZero()
        }
 }
 
@@ -1380,11 +1369,11 @@ func (p Prefix) Overlaps(o Prefix) bool {
        if p.ip.Is4() != o.ip.Is4() {
                return false
        }
-       var minBits int16
-       if p.bits < o.bits {
-               minBits = p.bits
+       var minBits int
+       if pb, ob := p.Bits(), o.Bits(); pb < ob {
+               minBits = pb
        } else {
-               minBits = o.bits
+               minBits = ob
        }
        if minBits == 0 {
                return true
@@ -1394,10 +1383,10 @@ func (p Prefix) Overlaps(o Prefix) bool {
        // so the Prefix call on the one that's already minBits serves to zero
        // out any remaining bits in IP.
        var err error
-       if p, err = p.ip.Prefix(int(minBits)); err != nil {
+       if p, err = p.ip.Prefix(minBits); err != nil {
                return false
        }
-       if o, err = o.ip.Prefix(int(minBits)); err != nil {
+       if o, err = o.ip.Prefix(minBits); err != nil {
                return false
        }
        return p.ip == o.ip
@@ -1427,7 +1416,7 @@ func (p Prefix) AppendTo(b []byte) []byte {
        }
 
        b = append(b, '/')
-       b = appendDecimal(b, uint8(p.bits))
+       b = appendDecimal(b, uint8(p.Bits()))
        return b
 }
 
@@ -1490,5 +1479,5 @@ func (p Prefix) String() string {
        if !p.IsValid() {
                return "invalid Prefix"
        }
-       return p.ip.String() + "/" + itoa.Itoa(int(p.bits))
+       return p.ip.String() + "/" + itoa.Itoa(p.Bits())
 }

diff --git a/src/net/netip/netip_pkg_test.go b/src/net/netip/netip_pkg_test.go
index 677f523e6d9667cfe18769da6dc13b5c62a8fd39..2c9a2e6a8cc148408cdd914a1f344b0019874d4d 100644 (file)
--- a/src/net/netip/netip_pkg_test.go
+++ b/src/net/netip/netip_pkg_test.go
@@ -24,30 +24,36 @@ func TestPrefixValid(t *testing.T) {
                ipp  Prefix
                want bool
        }{
-               {Prefix{v4, -2}, false},
-               {Prefix{v4, -1}, false},
-               {Prefix{v4, 0}, true},
-               {Prefix{v4, 32}, true},
-               {Prefix{v4, 33}, false},
+               {PrefixFrom(v4, -2), false},
+               {PrefixFrom(v4, -1), false},
+               {PrefixFrom(v4, 0), true},
+               {PrefixFrom(v4, 32), true},
+               {PrefixFrom(v4, 33), false},
 
-               {Prefix{v6, -2}, false},
-               {Prefix{v6, -1}, false},
-               {Prefix{v6, 0}, true},
-               {Prefix{v6, 32}, true},
-               {Prefix{v6, 128}, true},
-               {Prefix{v6, 129}, false},
+               {PrefixFrom(v6, -2), false},
+               {PrefixFrom(v6, -1), false},
+               {PrefixFrom(v6, 0), true},
+               {PrefixFrom(v6, 32), true},
+               {PrefixFrom(v6, 128), true},
+               {PrefixFrom(v6, 129), false},
 
-               {Prefix{Addr{}, -2}, false},
-               {Prefix{Addr{}, -1}, false},
-               {Prefix{Addr{}, 0}, false},
-               {Prefix{Addr{}, 32}, false},
-               {Prefix{Addr{}, 128}, false},
+               {PrefixFrom(Addr{}, -2), false},
+               {PrefixFrom(Addr{}, -1), false},
+               {PrefixFrom(Addr{}, 0), false},
+               {PrefixFrom(Addr{}, 32), false},
+               {PrefixFrom(Addr{}, 128), false},
        }
        for _, tt := range tests {
                got := tt.ipp.IsValid()
                if got != tt.want {
                        t.Errorf("(%v).IsValid() = %v want %v", tt.ipp, got, tt.want)
                }
+
+               // Test that there is only one invalid Prefix representation per Addr.
+               invalid := PrefixFrom(tt.ipp.Addr(), -1)
+               if !got && tt.ipp != invalid {
+                       t.Errorf("(%v == %v) = false, want true", tt.ipp, invalid)
+               }
        }
 }
 
@@ -167,11 +173,11 @@ func TestPrefixContains(t *testing.T) {
                {mustPrefix("::1/0"), Addr{}, false},
                {mustPrefix("1.2.3.4/0"), Addr{}, false},
                // invalid Prefix
-               {Prefix{mustIP("::1"), 129}, mustIP("::1"), false},
-               {Prefix{mustIP("1.2.3.4"), 33}, mustIP("1.2.3.4"), false},
-               {Prefix{Addr{}, 0}, mustIP("1.2.3.4"), false},
-               {Prefix{Addr{}, 32}, mustIP("1.2.3.4"), false},
-               {Prefix{Addr{}, 128}, mustIP("::1"), false},
+               {PrefixFrom(mustIP("::1"), 129), mustIP("::1"), false},
+               {PrefixFrom(mustIP("1.2.3.4"), 33), mustIP("1.2.3.4"), false},
+               {PrefixFrom(Addr{}, 0), mustIP("1.2.3.4"), false},
+               {PrefixFrom(Addr{}, 32), mustIP("1.2.3.4"), false},
+               {PrefixFrom(Addr{}, 128), mustIP("::1"), false},
                // wrong IP family
                {mustPrefix("::1/0"), mustIP("1.2.3.4"), false},
                {mustPrefix("1.2.3.4/0"), mustIP("::1"), false},