Skip to content

Commit

Permalink
ipam: fix azure ipam test panics due to shared pointers.
Browse files Browse the repository at this point in the history
pkg/ipam/types.(*InstanceMap).DeepCopy(...) will iterate for all instances/interfaces in order to copy the data.
However, unlike what the name suggests, underlying instance pkg/ipam/types.Interface pointers are copied and shared in the returned instance map.
In some cases, this case result in memory corruption issues resulting in confusing panics while running tests such as:

```
panic: runtime error: makeslice: cap out of range
goroutine 1366 [running]:
strconv.appendQuotedWith({0xc000576208, 0x0, 0x3f?}, {0x1000000, 0x100000001000000}, 0x22, 0x0, 0x0)
	/opt/hostedtoolcache/go/1.22.0/x64/src/strconv/quote.go:35 +0x85
strconv.AppendQuote(...)

...
```

Capturing such an event in a debugger you would see a AzureInterface struct such as this with the IP string memory being corrupt (likely due to an interleaved read/write) being passed to logrus causing a crash.

```
github.com/cilium/cilium/pkg/azure/types.AzureAddress {
	IP: "\x10\x01\x00\x00\x00\x00\x00\x00\x10\x01\x00\x00\x00\x00\x00\x007d�_\x02\b\b\x19\x00\x00\x00\x00\x00\x00\x00\x00�\x1f�\x03\x00\x00\x00\x00W�\b\x00\x00\x00\x00\x00\x00p \x03\x00\x00\x00\x00�qi\x03\x00\x00\x00\x00...+51559946186908214 more",
	Subnet: "subsys",
	State: "instanceID",}
```
This ensures that the revision interface is correctly deepcopied such that the underlying resource is also safely copied.

Fixed: #31059

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
  • Loading branch information
tommyp1ckles committed Mar 15, 2024
1 parent 9e011ea commit 1ca6141
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 0 deletions.
4 changes: 4 additions & 0 deletions pkg/alibabacloud/eni/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,10 @@ type ENI struct {
Tags map[string]string `json:"tags,omitempty"`
}

func (e *ENI) DeepCopyInterface() types.Interface {
return e.DeepCopy()
}

// InterfaceID returns the identifier of the interface
func (e *ENI) InterfaceID() string {
return e.NetworkInterfaceID
Expand Down
4 changes: 4 additions & 0 deletions pkg/aws/eni/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,10 @@ type ENI struct {
Tags map[string]string `json:"tags,omitempty"`
}

func (e *ENI) DeepCopyInterface() types.Interface {
return e.DeepCopy()
}

// InterfaceID returns the identifier of the interface
func (e *ENI) InterfaceID() string {
return e.ID
Expand Down
4 changes: 4 additions & 0 deletions pkg/azure/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,10 @@ type AzureInterface struct {
resourceGroup string `json:"-"`
}

func (a *AzureInterface) DeepCopyInterface() types.Interface {
return a.DeepCopy()
}

// SetID sets the Azure interface ID, as well as extracting other fields from
// the ID itself.
func (a *AzureInterface) SetID(id string) {
Expand Down
4 changes: 4 additions & 0 deletions pkg/ipam/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,9 @@ type Interface interface {
// ForeachAddress must iterate over all addresses of the interface and
// call fn for each address
ForeachAddress(instanceID string, fn AddressIterator) error

// DeepCopyInterface returns a deep copy of the underlying interface type.
DeepCopyInterface() Interface
}

// InterfaceRevision is the configurationr revision of a network interface. It
Expand Down Expand Up @@ -542,6 +545,7 @@ func (m *InstanceMap) DeepCopy() *InstanceMap {
c := NewInstanceMap()
m.ForeachInterface("", func(instanceID, interfaceID string, rev InterfaceRevision) error {
// c is not exposed yet, we can access it without locking it
rev.Resource = rev.Resource.DeepCopyInterface()
c.updateLocked(instanceID, rev)
return nil
})
Expand Down
17 changes: 17 additions & 0 deletions pkg/ipam/types/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,23 @@ type mockInterface struct {
pools map[string][]net.IP
}

func (m *mockInterface) DeepCopyInterface() Interface {
mc := &mockInterface{
id: m.id,
pools: map[string][]net.IP{},
}
for id, pool := range m.pools {
pc := make([]net.IP, 0, len(pool))
for _, ip := range pool {
ipc := net.IP{}
copy(ipc, ip)
pc = append(pc, ipc)
}
mc.pools[id] = pc
}
return mc
}

func (m *mockInterface) InterfaceID() string {
return m.id
}
Expand Down

0 comments on commit 1ca6141

Please sign in to comment.