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 JSON marshal/unmarshal inconsistency #35727

Open
elad opened this issue Nov 21, 2019 · 4 comments
Open

net: IPNet JSON marshal/unmarshal inconsistency #35727

elad opened this issue Nov 21, 2019 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@elad
Copy link

elad commented Nov 21, 2019

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

$ go version
go version go1.13.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/elad/Library/Caches/go-build"
GOENV="/Users/elad/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/elad/go"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13.4/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13.4/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/12/1cmrxkzd3mjdw85xz7jj36cr0000gn/T/go-build857110316=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Wrote code that uses net.ParseCIDR() to create a *net.IPNet, then uses json.Marshal() to serialize it to JSON, then uses json.Unmarshal() to deserialize it back to *net.IPNet.

Here's a link: https://play.golang.org/p/ryc7rp4KyiZ

Code just in case:

package main

import (
	"encoding/json"
	"net"
	"testing"
	
	"github.com/stretchr/testify/assert"
)

func TestMarshal(t *testing.T) {
	var err error

	_, cidrOut, err := net.ParseCIDR("192.168.0.0/16")
	assert.Nil(t, err, "net.ParseCIDR")

	cidrJson, err := json.Marshal(cidrOut)
	assert.Nil(t, err, "json.Marshal")

	var cidrIn *net.IPNet
	err = json.Unmarshal(cidrJson, &cidrIn)
	assert.Nil(t, err, "json.Unmarshal")
	
	assert.Equal(t, cidrOut, cidrIn, "CIDR value differs")
	assert.Equal(t, len(cidrOut.IP), len(cidrIn.IP), "IP length differs")
}

What did you expect to see?

I expected that the original and deserialized values will be equal.

What did you see instead?

I saw that the original and deserialized values are different.

Specifically, I saw that the original value as returned from net.ParseCIDR() uses 4-byte representation and the value as deserialized by json.Unmarshal() uses 16-byte representation. The values might be semantically equivalent but the internal byte representation is different. Note that this value (net.IPNet.IP) is managed internally and not used at all by the calling code.

I think this might be because net.ParseCIDR() masks the net.IPNet.IP field with a mask that is limited by the IP address length and by default assumes IPv4, so 4-byte representation. The result is that for an IPv4 CIDR, the internal net.IPNet.IP will always be in 4-byte representation. The IP.UnmarshalText() function uses net.ParseIP(), though, which calls parseIPv4(), which in turn uses net.IPv4() to construct the net.IP that is returned. But net.IPv4() uses 16-byte representation ("v4-in-v6 prefix").

I think this is not ideal since in order to get semantics where input is equal to output one has to add code as below, which alters internal data structures:

v4 := cidr.IP.To4(); if v4 != nil {
    cidr.IP = v4
}

If this is indeed considered a problem, a naive suggestion would be to add an unmarshal function for net.IPNet that uses net.ParseCIDR() and thus retaining semantics. I don't know if that's viable though or what unforeseen effects this might have.

@wI2L
Copy link
Contributor

wI2L commented Nov 21, 2019

/cc @mvdan

@mvdan mvdan changed the title net.IPNet JSON marshal/unmarshal inconsistency net: IPNet JSON marshal/unmarshal inconsistency Nov 21, 2019
@toothrot toothrot added this to the Backlog milestone Nov 25, 2019
@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 25, 2019
@toothrot
Copy link
Contributor

Also /cc @mikioh

@elad
Copy link
Author

elad commented Nov 25, 2019

Another way to reproduce the problem, without *net.IPNet, is to call IP.To4() and then do the serialization and deserialization.

Playground: https://play.golang.org/p/VgWt3PtRN8o

@yevgenypats
Copy link

yevgenypats commented Nov 15, 2022

Yep, got hit by that as well, had to implement custom marshal and a wrapper struct as marshal/unmarshal doesn't really work.

yevgenypats added a commit to cloudquery/plugin-sdk that referenced this issue Nov 15, 2022
yevgenypats added a commit to cloudquery/plugin-sdk that referenced this issue Nov 15, 2022
yevgenypats added a commit to cloudquery/plugin-sdk that referenced this issue Nov 15, 2022
yevgenypats added a commit to cloudquery/plugin-sdk that referenced this issue Nov 15, 2022
kodiakhq bot pushed a commit to cloudquery/plugin-sdk that referenced this issue Nov 15, 2022
Fix nasty bug triggered by Go bug

golang/go#35727

@shimonp21  can you please confirm this solves the bug triggered by the k8s cidr plugin.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

4 participants