[go] net/netip: fix invalid representation of Prefix

15 views
Skip to first unread message

Joseph Tsai (Gerrit)

unread,
Aug 20, 2022, 3:09:06 PM8/20/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Joseph Tsai has uploaded this change for review.

View Change

net/netip: fix invalid representation of Prefix

For a given Addr, make it such that there is exactly one invalid representation.
This allows invalid representations to also be 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.
This simplifies IsValid to simply checking whether bitsPlusOne is non-zero.

Fixes #54525

Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
---
M src/net/netip/netip.go
M src/net/netip/netip_pkg_test.go
2 files changed, 71 insertions(+), 55 deletions(-)

diff --git a/src/net/netip/netip.go b/src/net/netip/netip.go
index b5d55ac..9738334 100644
--- a/src/net/netip/netip.go
+++ b/src/net/netip/netip.go
@@ -1235,20 +1235,11 @@
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.
//
@@ -1258,13 +1249,14 @@
// 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
+ if !ip.isZero() && bits >= 0 && bits <= ip.BitLen() {
+ bits++
+ } else {
+ bits = 0
}
- b16 := int16(bits)
return Prefix{
- ip: ip.withoutZone(),
- bits: b16,
+ ip: ip.withoutZone(),
+ bitsPlusOne: uint8(bits),
}
}

@@ -1274,17 +1266,17 @@
// 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() && int(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",
@@ -1337,10 +1329,8 @@
//
// 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.
@@ -1366,12 +1356,12 @@
// 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()
}
}

@@ -1390,11 +1380,11 @@
if p.ip.Is4() != o.ip.Is4() {
return false
}
- var minBits int16
- if p.bits < o.bits {
- minBits = p.bits
+ var minBits int
+ if p.Bits() < o.Bits() {
+ minBits = p.Bits()
} else {
- minBits = o.bits
+ minBits = o.Bits()
}
if minBits == 0 {
return true
@@ -1404,10 +1394,10 @@
// 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
@@ -1437,7 +1427,7 @@
}

b = append(b, '/')
- b = appendDecimal(b, uint8(p.bits))
+ b = appendDecimal(b, uint8(p.Bits()))
return b
}

@@ -1500,5 +1490,5 @@
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 677f523..2c9a2e6 100644
--- a/src/net/netip/netip_pkg_test.go
+++ b/src/net/netip/netip_pkg_test.go
@@ -24,30 +24,36 @@
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 @@
{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},

To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: go
Gerrit-Branch: master
Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
Gerrit-Change-Number: 425035
Gerrit-PatchSet: 1
Gerrit-Owner: Joseph Tsai <joe...@digital-static.net>
Gerrit-MessageType: newchange

Joseph Tsai (Gerrit)

unread,
Aug 20, 2022, 3:12:49 PM8/20/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Joseph Tsai uploaded patch set #2 to this change.

View Change

net/netip: fix invalid representation of Prefix

For a given Addr, ensure there is exactly one invalid representation.
This allows invalid representations to also 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.
This simplifies IsValid to just checking whether bitsPlusOne is non-zero.


Fixes #54525

Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
---
M src/net/netip/netip.go
M src/net/netip/netip_pkg_test.go
2 files changed, 71 insertions(+), 55 deletions(-)

To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: go
Gerrit-Branch: master
Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
Gerrit-Change-Number: 425035
Gerrit-PatchSet: 2
Gerrit-Owner: Joseph Tsai <joe...@digital-static.net>
Gerrit-MessageType: newpatchset

Joseph Tsai (Gerrit)

unread,
Aug 20, 2022, 3:15:38 PM8/20/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Joseph Tsai uploaded patch set #3 to this change.

View Change

net/netip: fix invalid representation of Prefix

For a given Addr, ensure there is exactly one invalid representation.
This allows invalid representations to also 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
---
M src/net/netip/netip.go
M src/net/netip/netip_pkg_test.go
2 files changed, 71 insertions(+), 55 deletions(-)

To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: go
Gerrit-Branch: master
Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
Gerrit-Change-Number: 425035
Gerrit-PatchSet: 3

Joseph Tsai (Gerrit)

unread,
Aug 20, 2022, 3:19:03 PM8/20/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Joseph Tsai uploaded patch set #4 to this change.

View Change

net/netip: fix invalid representation of Prefix

For a given Addr, ensure there is exactly one invalid representation.
This allows invalid representations to also 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
---
M src/net/netip/netip.go
M src/net/netip/netip_pkg_test.go
2 files changed, 70 insertions(+), 55 deletions(-)

To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: go
Gerrit-Branch: master
Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
Gerrit-Change-Number: 425035
Gerrit-PatchSet: 4

Joseph Tsai (Gerrit)

unread,
Aug 20, 2022, 3:20:13 PM8/20/22
to goph...@pubsubhelper.golang.org, Brad Fitzpatrick, Ian Lance Taylor, Damien Neil, Gopher Robot, golang-co...@googlegroups.com

Attention is currently required from: Brad Fitzpatrick, Damien Neil, Ian Lance Taylor.

Patch set 4:Run-TryBot +1Auto-Submit +1

View Change

    To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: go
    Gerrit-Branch: master
    Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
    Gerrit-Change-Number: 425035
    Gerrit-PatchSet: 4
    Gerrit-Owner: Joseph Tsai <joe...@digital-static.net>
    Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
    Gerrit-Reviewer: Damien Neil <dn...@google.com>
    Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
    Gerrit-Reviewer: Joseph Tsai <joe...@digital-static.net>
    Gerrit-CC: Gopher Robot <go...@golang.org>
    Gerrit-Attention: Brad Fitzpatrick <brad...@golang.org>
    Gerrit-Attention: Damien Neil <dn...@google.com>
    Gerrit-Attention: Ian Lance Taylor <ia...@golang.org>
    Gerrit-Comment-Date: Sat, 20 Aug 2022 19:20:10 +0000
    Gerrit-HasComments: No
    Gerrit-Has-Labels: Yes
    Gerrit-MessageType: comment

    Joseph Tsai (Gerrit)

    unread,
    Aug 20, 2022, 3:24:47 PM8/20/22
    to goph...@pubsubhelper.golang.org, Brad Fitzpatrick, Ian Lance Taylor, Damien Neil, Gopher Robot, golang-co...@googlegroups.com

    Attention is currently required from: Brad Fitzpatrick, Damien Neil, Ian Lance Taylor.

    View Change

    1 comment:

    • File src/net/netip/netip.go:

      • Patch Set #4, Line 1253: if !ip.isZero() && bits >= 0 && bits <= ip.BitLen() {

        If bits is invalid, it is debatable whether we should even preserve the Addr.
        This CL currently preserves the Addr (as did the old behavior).

        An argument for dropping a valid Addr for invalid bits is that String, (Un)MarshalText, and (Un)MarshalBinary does not preserve the Addr round-trip.

    To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: go
    Gerrit-Branch: master
    Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
    Gerrit-Change-Number: 425035
    Gerrit-PatchSet: 4
    Gerrit-Owner: Joseph Tsai <joe...@digital-static.net>
    Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
    Gerrit-Reviewer: Damien Neil <dn...@google.com>
    Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
    Gerrit-Reviewer: Joseph Tsai <joe...@digital-static.net>
    Gerrit-CC: Gopher Robot <go...@golang.org>
    Gerrit-Attention: Brad Fitzpatrick <brad...@golang.org>
    Gerrit-Attention: Damien Neil <dn...@google.com>
    Gerrit-Attention: Ian Lance Taylor <ia...@golang.org>
    Gerrit-Comment-Date: Sat, 20 Aug 2022 19:24:43 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Gerrit-MessageType: comment

    Joseph Tsai (Gerrit)

    unread,
    Aug 20, 2022, 3:28:28 PM8/20/22
    to goph...@pubsubhelper.golang.org, Josh Bleecher Snyder, Matt Layher, David Anderson, Brad Fitzpatrick, Ian Lance Taylor, Damien Neil, Gopher Robot, golang-co...@googlegroups.com

    Attention is currently required from: Brad Fitzpatrick, Damien Neil, Ian Lance Taylor.

    View Change

    1 comment:

    • File src/net/netip/netip.go:

    To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: go
    Gerrit-Branch: master
    Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
    Gerrit-Change-Number: 425035
    Gerrit-PatchSet: 4
    Gerrit-Owner: Joseph Tsai <joe...@digital-static.net>
    Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
    Gerrit-Reviewer: Damien Neil <dn...@google.com>
    Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
    Gerrit-Reviewer: Joseph Tsai <joe...@digital-static.net>
    Gerrit-CC: David Anderson <da...@natulte.net>
    Gerrit-CC: Gopher Robot <go...@golang.org>
    Gerrit-CC: Josh Bleecher Snyder <josh...@gmail.com>
    Gerrit-CC: Matt Layher <mdla...@gmail.com>
    Gerrit-Attention: Brad Fitzpatrick <brad...@golang.org>
    Gerrit-Attention: Damien Neil <dn...@google.com>
    Gerrit-Attention: Ian Lance Taylor <ia...@golang.org>
    Gerrit-Comment-Date: Sat, 20 Aug 2022 19:28:24 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Joseph Tsai <joe...@digital-static.net>
    Gerrit-MessageType: comment

    Joseph Tsai (Gerrit)

    unread,
    Aug 20, 2022, 3:38:40 PM8/20/22
    to goph...@pubsubhelper.golang.org, Gopher Robot, Josh Bleecher Snyder, Matt Layher, David Anderson, Brad Fitzpatrick, Ian Lance Taylor, Damien Neil, golang-co...@googlegroups.com

    Attention is currently required from: Brad Fitzpatrick, Damien Neil, Ian Lance Taylor.

    View Change

    1 comment:

    • File src/net/netip/netip.go:

      • Some related discussion on https://github. […]

        BTW, this CL is not blocked on figuring this out. Whatever we do, we'll want to be consistent in behavior for both Prefix and AddrPort.

    To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: go
    Gerrit-Branch: master
    Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
    Gerrit-Change-Number: 425035
    Gerrit-PatchSet: 4
    Gerrit-Owner: Joseph Tsai <joe...@digital-static.net>
    Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
    Gerrit-Reviewer: Damien Neil <dn...@google.com>
    Gerrit-Reviewer: Gopher Robot <go...@golang.org>
    Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
    Gerrit-Reviewer: Joseph Tsai <joe...@digital-static.net>
    Gerrit-CC: David Anderson <da...@natulte.net>
    Gerrit-CC: Josh Bleecher Snyder <josh...@gmail.com>
    Gerrit-CC: Matt Layher <mdla...@gmail.com>
    Gerrit-Attention: Brad Fitzpatrick <brad...@golang.org>
    Gerrit-Attention: Damien Neil <dn...@google.com>
    Gerrit-Attention: Ian Lance Taylor <ia...@golang.org>
    Gerrit-Comment-Date: Sat, 20 Aug 2022 19:38:36 +0000

    Ian Lance Taylor (Gerrit)

    unread,
    Jan 23, 2023, 7:19:03 PM1/23/23
    to Joseph Tsai, goph...@pubsubhelper.golang.org, Ian Lance Taylor, Gopher Robot, Josh Bleecher Snyder, Matt Layher, David Anderson, Brad Fitzpatrick, Damien Neil, golang-co...@googlegroups.com

    Attention is currently required from: Brad Fitzpatrick, Joseph Tsai.

    Patch set 4:Code-Review +2

    View Change

    1 comment:

    • File src/net/netip/netip.go:

      • Patch Set #4, Line 1383: if p.Bits() < o.Bits() {

        if pb, ob := p.Bits(), o.Bits(); pb < ob {
        minBits = pb
        else {
        minBits = ob
        }

    To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: go
    Gerrit-Branch: master
    Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
    Gerrit-Change-Number: 425035
    Gerrit-PatchSet: 4
    Gerrit-Owner: Joseph Tsai <joe...@digital-static.net>
    Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
    Gerrit-Reviewer: Damien Neil <dn...@google.com>
    Gerrit-Reviewer: Gopher Robot <go...@golang.org>
    Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
    Gerrit-Reviewer: Joseph Tsai <joe...@digital-static.net>
    Gerrit-CC: David Anderson <da...@natulte.net>
    Gerrit-CC: Josh Bleecher Snyder <josh...@gmail.com>
    Gerrit-CC: Matt Layher <mdla...@gmail.com>
    Gerrit-Attention: Joseph Tsai <joe...@digital-static.net>
    Gerrit-Attention: Brad Fitzpatrick <brad...@golang.org>
    Gerrit-Comment-Date: Tue, 24 Jan 2023 00:18:57 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Gerrit-MessageType: comment

    Ian Lance Taylor (Gerrit)

    unread,
    Jan 23, 2023, 7:19:13 PM1/23/23
    to Joseph Tsai, goph...@pubsubhelper.golang.org, Ian Lance Taylor, Gopher Robot, Josh Bleecher Snyder, Matt Layher, David Anderson, Brad Fitzpatrick, Damien Neil, golang-co...@googlegroups.com

    Attention is currently required from: Brad Fitzpatrick, Joseph Tsai.

    Patch set 4:Code-Review +1

    View Change

      To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: go
      Gerrit-Branch: master
      Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
      Gerrit-Change-Number: 425035
      Gerrit-PatchSet: 4
      Gerrit-Owner: Joseph Tsai <joe...@digital-static.net>
      Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
      Gerrit-Reviewer: Damien Neil <dn...@google.com>
      Gerrit-Reviewer: Gopher Robot <go...@golang.org>
      Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
      Gerrit-Reviewer: Ian Lance Taylor <ia...@google.com>
      Gerrit-Reviewer: Joseph Tsai <joe...@digital-static.net>
      Gerrit-CC: David Anderson <da...@natulte.net>
      Gerrit-CC: Josh Bleecher Snyder <josh...@gmail.com>
      Gerrit-CC: Matt Layher <mdla...@gmail.com>
      Gerrit-Attention: Joseph Tsai <joe...@digital-static.net>
      Gerrit-Attention: Brad Fitzpatrick <brad...@golang.org>
      Gerrit-Comment-Date: Tue, 24 Jan 2023 00:19:07 +0000

      Joseph Tsai (Gerrit)

      unread,
      Jan 24, 2023, 3:38:13 PM1/24/23
      to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

      Attention is currently required from: Brad Fitzpatrick, Joseph Tsai.

      Joseph Tsai uploaded patch set #5 to this change.

      View Change

      The following approvals got outdated and were removed: Auto-Submit+1 by Joseph Tsai, Run-TryBot+1 by Joseph Tsai, TryBot-Result+1 by Gopher Robot

      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
      ---
      M src/net/netip/netip.go
      M src/net/netip/netip_pkg_test.go
      2 files changed, 70 insertions(+), 55 deletions(-)

      To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: go
      Gerrit-Branch: master
      Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
      Gerrit-Change-Number: 425035
      Gerrit-PatchSet: 5
      Gerrit-Owner: Joseph Tsai <joe...@digital-static.net>
      Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
      Gerrit-Reviewer: Damien Neil <dn...@google.com>
      Gerrit-Reviewer: Gopher Robot <go...@golang.org>
      Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
      Gerrit-Reviewer: Ian Lance Taylor <ia...@google.com>
      Gerrit-Reviewer: Joseph Tsai <joe...@digital-static.net>
      Gerrit-CC: David Anderson <da...@natulte.net>
      Gerrit-CC: Josh Bleecher Snyder <josh...@gmail.com>
      Gerrit-CC: Matt Layher <mdla...@gmail.com>
      Gerrit-Attention: Joseph Tsai <joe...@digital-static.net>
      Gerrit-Attention: Brad Fitzpatrick <brad...@golang.org>
      Gerrit-MessageType: newpatchset

      Joseph Tsai (Gerrit)

      unread,
      Jan 24, 2023, 3:38:46 PM1/24/23
      to goph...@pubsubhelper.golang.org, Ian Lance Taylor, Gopher Robot, Josh Bleecher Snyder, Matt Layher, David Anderson, Brad Fitzpatrick, Damien Neil, golang-co...@googlegroups.com

      Attention is currently required from: Brad Fitzpatrick.

      Patch set 6:Run-TryBot +1Auto-Submit +1

      View Change

      1 comment:

      • File src/net/netip/netip.go:

        • if pb, ob := p.Bits(), o.Bits(); pb < ob { […]

          Done

      To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: go
      Gerrit-Branch: master
      Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
      Gerrit-Change-Number: 425035
      Gerrit-PatchSet: 6
      Gerrit-Owner: Joseph Tsai <joe...@digital-static.net>
      Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
      Gerrit-Reviewer: Damien Neil <dn...@google.com>
      Gerrit-Reviewer: Gopher Robot <go...@golang.org>
      Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
      Gerrit-Reviewer: Ian Lance Taylor <ia...@google.com>
      Gerrit-Reviewer: Joseph Tsai <joe...@digital-static.net>
      Gerrit-CC: David Anderson <da...@natulte.net>
      Gerrit-CC: Josh Bleecher Snyder <josh...@gmail.com>
      Gerrit-CC: Matt Layher <mdla...@gmail.com>
      Gerrit-Attention: Brad Fitzpatrick <brad...@golang.org>
      Gerrit-Comment-Date: Tue, 24 Jan 2023 20:38:41 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      Comment-In-Reply-To: Ian Lance Taylor <ia...@golang.org>
      Gerrit-MessageType: comment

      Joseph Tsai (Gerrit)

      unread,
      Jan 24, 2023, 3:40:42 PM1/24/23
      to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

      Attention is currently required from: Brad Fitzpatrick, Joseph Tsai.

      Joseph Tsai uploaded patch set #7 to this change.

      View Change

      The following approvals got outdated and were removed: Auto-Submit+1 by Joseph Tsai, Run-TryBot+1 by Joseph Tsai

      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
      ---
      M src/net/netip/netip.go
      M src/net/netip/netip_pkg_test.go
      2 files changed, 70 insertions(+), 55 deletions(-)

      To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: go
      Gerrit-Branch: master
      Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
      Gerrit-Change-Number: 425035
      Gerrit-PatchSet: 7
      Gerrit-Owner: Joseph Tsai <joe...@digital-static.net>
      Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
      Gerrit-Reviewer: Damien Neil <dn...@google.com>
      Gerrit-Reviewer: Gopher Robot <go...@golang.org>
      Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
      Gerrit-Reviewer: Ian Lance Taylor <ia...@google.com>
      Gerrit-Reviewer: Joseph Tsai <joe...@digital-static.net>
      Gerrit-CC: David Anderson <da...@natulte.net>
      Gerrit-CC: Josh Bleecher Snyder <josh...@gmail.com>
      Gerrit-CC: Matt Layher <mdla...@gmail.com>

      Joseph Tsai (Gerrit)

      unread,
      Jan 24, 2023, 3:40:55 PM1/24/23
      to goph...@pubsubhelper.golang.org, Ian Lance Taylor, Gopher Robot, Josh Bleecher Snyder, Matt Layher, David Anderson, Brad Fitzpatrick, Damien Neil, golang-co...@googlegroups.com

      Attention is currently required from: Brad Fitzpatrick.

      Patch set 7:Run-TryBot +1Auto-Submit +1

      View Change

        To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

        Gerrit-Project: go
        Gerrit-Branch: master
        Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
        Gerrit-Change-Number: 425035
        Gerrit-PatchSet: 7
        Gerrit-Owner: Joseph Tsai <joe...@digital-static.net>
        Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
        Gerrit-Reviewer: Damien Neil <dn...@google.com>
        Gerrit-Reviewer: Gopher Robot <go...@golang.org>
        Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
        Gerrit-Reviewer: Ian Lance Taylor <ia...@google.com>
        Gerrit-Reviewer: Joseph Tsai <joe...@digital-static.net>
        Gerrit-CC: David Anderson <da...@natulte.net>
        Gerrit-CC: Josh Bleecher Snyder <josh...@gmail.com>
        Gerrit-CC: Matt Layher <mdla...@gmail.com>
        Gerrit-Attention: Brad Fitzpatrick <brad...@golang.org>
        Gerrit-Comment-Date: Tue, 24 Jan 2023 20:40:51 +0000

        Joseph Tsai (Gerrit)

        unread,
        Feb 2, 2023, 6:28:46 AM2/2/23
        to goph...@pubsubhelper.golang.org, Gopher Robot, Ian Lance Taylor, Josh Bleecher Snyder, Matt Layher, David Anderson, Brad Fitzpatrick, Damien Neil, golang-co...@googlegroups.com

        Attention is currently required from: Brad Fitzpatrick.

        View Change

        1 comment:

        • File src/net/netip/netip.go:

          • Patch Set #4, Line 1253: if !ip.isZero() && bits >= 0 && bits <= ip.BitLen() {

            BTW, this CL is not blocked on figuring this out. […]

            Ack

        To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

        Gerrit-Project: go
        Gerrit-Branch: master
        Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
        Gerrit-Change-Number: 425035
        Gerrit-PatchSet: 7
        Gerrit-Owner: Joseph Tsai <joe...@digital-static.net>
        Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
        Gerrit-Reviewer: Damien Neil <dn...@google.com>
        Gerrit-Reviewer: Gopher Robot <go...@golang.org>
        Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
        Gerrit-Reviewer: Ian Lance Taylor <ia...@google.com>
        Gerrit-Reviewer: Joseph Tsai <joe...@digital-static.net>
        Gerrit-CC: David Anderson <da...@natulte.net>
        Gerrit-CC: Josh Bleecher Snyder <josh...@gmail.com>
        Gerrit-CC: Matt Layher <mdla...@gmail.com>
        Gerrit-Attention: Brad Fitzpatrick <brad...@golang.org>
        Gerrit-Comment-Date: Thu, 02 Feb 2023 11:28:40 +0000

        Michael Knyszek (Gerrit)

        unread,
        Feb 2, 2023, 10:37:11 AM2/2/23
        to Joseph Tsai, goph...@pubsubhelper.golang.org, Gopher Robot, Ian Lance Taylor, Josh Bleecher Snyder, Matt Layher, David Anderson, Brad Fitzpatrick, Damien Neil, golang-co...@googlegroups.com

        Attention is currently required from: Brad Fitzpatrick, Joseph Tsai.

        Patch set 7:Code-Review +1

        View Change

          To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

          Gerrit-Project: go
          Gerrit-Branch: master
          Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
          Gerrit-Change-Number: 425035
          Gerrit-PatchSet: 7
          Gerrit-Owner: Joseph Tsai <joe...@digital-static.net>
          Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
          Gerrit-Reviewer: Damien Neil <dn...@google.com>
          Gerrit-Reviewer: Gopher Robot <go...@golang.org>
          Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
          Gerrit-Reviewer: Ian Lance Taylor <ia...@google.com>
          Gerrit-Reviewer: Joseph Tsai <joe...@digital-static.net>
          Gerrit-Reviewer: Michael Knyszek <mkny...@google.com>
          Gerrit-CC: David Anderson <da...@natulte.net>
          Gerrit-CC: Josh Bleecher Snyder <josh...@gmail.com>
          Gerrit-CC: Matt Layher <mdla...@gmail.com>
          Gerrit-Attention: Joseph Tsai <joe...@digital-static.net>
          Gerrit-Attention: Brad Fitzpatrick <brad...@golang.org>
          Gerrit-Comment-Date: Thu, 02 Feb 2023 15:37:06 +0000

          Gopher Robot (Gerrit)

          unread,
          Feb 2, 2023, 10:37:24 AM2/2/23
          to Joseph Tsai, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Michael Knyszek, Ian Lance Taylor, Josh Bleecher Snyder, Matt Layher, David Anderson, Brad Fitzpatrick, Damien Neil, golang-co...@googlegroups.com

          Gopher Robot submitted this change.

          View Change



          4 is the latest approved patch-set.
          The change was submitted with unreviewed changes in the following files:

          ```
          The name of the file: src/net/netip/netip.go
          Insertions: 4, Deletions: 4.

          The diff is too large to show. Please review the diff.
          ```

          Approvals: Ian Lance Taylor: Looks good to me, but someone else must approve Joseph Tsai: Run TryBots; Automatically submit change Ian Lance Taylor: Looks good to me, approved Michael Knyszek: Looks good to me, but someone else must approve Gopher Robot: TryBots succeeded
          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 <joe...@digital-static.net>
          Reviewed-by: Michael Knyszek <mkny...@google.com>
          Reviewed-by: Ian Lance Taylor <ia...@golang.org>
          TryBot-Result: Gopher Robot <go...@golang.org>
          Reviewed-by: Ian Lance Taylor <ia...@google.com>
          Auto-Submit: Joseph Tsai <joe...@digital-static.net>

          ---
          M src/net/netip/netip.go
          M src/net/netip/netip_pkg_test.go
          2 files changed, 77 insertions(+), 55 deletions(-)

          diff --git a/src/net/netip/netip.go b/src/net/netip/netip.go
          index ec9266d..aa700f4 100644
          --- a/src/net/netip/netip.go
          +++ b/src/net/netip/netip.go
          @@ -1225,20 +1225,11 @@

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

          // 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 @@

          // 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 @@

          //
          // 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 @@

          // 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 @@

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

          // 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 @@

          }

          b = append(b, '/')
          - b = appendDecimal(b, uint8(p.bits))
          + b = appendDecimal(b, uint8(p.Bits()))
          return b
          }

          @@ -1490,5 +1479,5 @@

          To view, visit change 425035. To unsubscribe, or for help writing mail filters, visit settings.

          Gerrit-Project: go
          Gerrit-Branch: master
          Gerrit-Change-Id: I9244cae2fd160cc9c81d007866992df2e422d3b9
          Gerrit-Change-Number: 425035
          Gerrit-PatchSet: 8
          Gerrit-Owner: Joseph Tsai <joe...@digital-static.net>
          Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
          Gerrit-Reviewer: Damien Neil <dn...@google.com>
          Gerrit-Reviewer: Gopher Robot <go...@golang.org>
          Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
          Gerrit-Reviewer: Ian Lance Taylor <ia...@google.com>
          Gerrit-Reviewer: Joseph Tsai <joe...@digital-static.net>
          Gerrit-Reviewer: Michael Knyszek <mkny...@google.com>
          Gerrit-CC: David Anderson <da...@natulte.net>
          Gerrit-CC: Josh Bleecher Snyder <josh...@gmail.com>
          Gerrit-CC: Matt Layher <mdla...@gmail.com>
          Gerrit-MessageType: merged
          Reply all
          Reply to author
          Forward
          0 new messages