Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: IPNet.String() returns an unexpected string value '<nil>' #39516

Closed
colinmcintosh opened this issue Jun 11, 2020 · 4 comments
Closed

net: IPNet.String() returns an unexpected string value '<nil>' #39516

colinmcintosh opened this issue Jun 11, 2020 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@colinmcintosh
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.13.7 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Not applicable.

What did you do?

When calling the .String() method on net.IPNet the method will return a string containing the text "<nil>" rather than a zero-value string or a nil pointer. This arbitrary string behavior is not documented and was an unexpected response to indicate an inability to string the IPNet data.

Can this be changed to return a proper string zero-value?

package main

import (
	"fmt"
	"net"
)

func main() {
	ip := new(net.IPNet)
	ip.IP = []byte("127.0.0.1")
	// ip.Mask not set
	fmt.Printf("%T: %s\n", ip.String(), ip.String()) // string: <nil>
}

What did you expect to see?

An empty string.

What did you see instead?

An arbitrary string containing the text "<nil>".

@antong
Copy link
Contributor

antong commented Jun 11, 2020

Thank you for the report and for the clear code example!

I don't think an empty string would be any less arbitrary. I might even argue it could be ok for String() to panic for an uninitialized IPNet (nil fields). Also, the string "<nil>" is not that uncommon for things similar to this. fmt for example will output "<nil>" for nil pointers and for String() pointer receivers that panic because of dereferencing a nil pointer (Example: https://play.golang.org/p/f2gNtD7W8Pl). So, I would say returning "<nil>" is an option as good as any, and the function is good as it is.

How did you end up with an unitialized IPNet in the first place? The example code you provided looks like a mistake as it assigns to the IP the bytes of the string representation of 127.0.0.1 ([]byte("127.0.0.1"), when it should be []byte{127,0,0,1} or even clearer, IPv4(127, 0, 0, 1). Perhaps the net.ParseCIDR() function is what you are looking for? Using that, you would not end up with a partially initialized IPNet.

@toothrot toothrot changed the title net.IPNet.String() returns an unexpected string value '<nil>' net: IPNet.String() returns an unexpected string value '<nil>' Jun 12, 2020
@toothrot toothrot added this to the Backlog milestone Jun 12, 2020
@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 12, 2020
@toothrot
Copy link
Contributor

I agree with @antong's helpful response here. I'm closing this, as I don't believe this is an issue with the standard library. Please comment if I am mistaken.

@cben
Copy link

cben commented Dec 21, 2020

  1. See comments under the question https://stackoverflow.com/questions/46985008/how-to-check-a-nil-value-of-pointer-to-struct-net-ipnet-in-golang#comment80917301_46985008 for an example how the particular choice of <nil> is confusing people on the distinction between nil pointer to struct vs zero value of struct:

    Thats because b.IP isn't nil. – tkausl

    so what is the real value of b.IP? printing it out as in the playground showed that its value is nil. Pardon that I am a Go newbie. – jap

    I'd say it's misleading not only to newbies, to anybody debugging code using IPNet type.
    WDYT about choosing some other representation like <zero> or <invalid>? Anything that avoids the word "nil".

  2. Do you generally consider zero value of IPNet{} invalid? This is unclear from docs.
    ParseCIDR() will never return it, but the docs don't say you're only supposed to use that as constructor; it's a struct with public fields, so default assumption of readers will be it's valid.
    IPNet.String() doc never says it might return <nil>.

    The reason it's invalid is because the zero value of IP contains an empty byte slice. It's not even clear whether it's an IPv4 or IPv6.
    IP.String() docs do say it returns "<nil>, if ip has length 0". Which technically makes sense because the zero value is a nil slice.
    (It'd also return <nil> for a non-nil len-0 slice, which is a mild lie but I don't object — I love nil slice / 0-len slice equivalence, and anyway it's documented behavior.)

  3. While not a valid address, the zero value of IPNet is useful to signal lack of value!
    For example, pflag.IPNet implements command-line flags that parse a CIDR into an IPNet struct; if the flag was not provided, the var remains at zero value.

    Problem: there is no publicly recommended API to detect whether an IPNet is the zero value.
    I now have code checking cidr.String() == "<nil>" and I might not be the only one.
    But it's a bad idea as such code will break if the <nil> represantation is ever replaced :-(

I don't know if formally this falls under Go 1 compatibility promise. For IPNet.String, this <nil> behavior is not documented yet 🤷
But in any case, I think a good first step would be to add a method checking whether IPNet is zero or valid, only later consider changing vs freezing the representation?

  • Hmm, calling it .IsZero() (inspired by Time.IsZero()) might be confusing because one could expect it to return true for 0.0.0.0/0 or ::0/0.
  • If we call it .Valid(), semantics is also unclear. Is a /0 CIDR "valid"? Is it valid when IPMask is not canonical form (ones followed by zeros)?
  • Maybe .Empty()? Could also implement .Empty() only on IP, and document that the recommended way to check IPNet is testing .IP.Empty().
  • What are the options docs could recommend without adding a method?
    • Directly compare to empty value? Won't work:
      invalid operation: cidr == net.IPNet literal (struct containing net.IP cannot be compared)
      
    • Advise people to test len(cidr.IP) == 0?

WDYT?

@cben
Copy link

cben commented Dec 21, 2020

Another consistent position to take could be that IPNet is just a container, with no opinion on validity of its content, and that IPNet.String() should always dump all the data it has.
A practical benefit of such position is giving maximum information to help one debug why they got non-sensical data.

  • It already offers a representation for non-canonical masks ("198.51.100.0/c000ff00").
  • Given empty .IP but non-trivial mask, should return not merely <nil> but <nil>/13.
  • Oh, mask is also a slice! Given valid .IP but nil mask should return 1.2.3.4/<nil>.
  • For zero value of both IP and mask, I guess it will return <nil>/<nil>?
  • What about 4-byte mask and an IPv6 address (that is not just a mapped IPv4 address)?
  • What about 16-byte mask and IPv4 address?
  • What about address and/or mask whose length != 4 and != 16? Could still fall back to printing in hex. However nobody cares 😉

Another benefit here is that the word "nil" will no longer be confusing for a nil *IPNet, it's clearly just the the IP and/or mask part is actually nil (or well, a 0-len slice).

Again, not sure this is OK with compatibility promise (de jure not sure because not documented, de facto likely to break something for somebody)...

(None of this is enough to make ParseCIDR() round-trip any String() value, but that is not a goal.)

@golang golang locked and limited conversation to collaborators Dec 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants