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.

Sign up for GitHub

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

Replace unsafe.Slice with memory copying to avoid potential fault memory issue #6664

Merged
merged 1 commit into from
Sep 14, 2024
Merged

Replace unsafe.Slice with memory copying to avoid potential fault memory issue #6664

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion pkg/agent/util/syscall/syscall_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,16 @@ func (n *netIO) ListIPForwardRows(family uint16) ([]MibIPForwardRow, error) {
if err != nil {
return nil, os.NewSyscallError("iphlpapi.GetIpForwardTable", err)
}
return unsafe.Slice(&table.Table[0], table.NumEntries), nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know the only issue with the previous code is that we were not making a copy of the slice prior to returning, while at the same time freeing the underlying memory by calling n.freeMibTable(unsafe.Pointer(table)).

The Wireguard Windows code has a simpler implementation of this, which still uses unsafe.Slice. It achieves correctness by making a proper copy of the slice: https://github.com/WireGuard/wireguard-windows/blob/e70799b1440690e7d4140bffc7c73baf903c7b54/tunnel/winipcfg/winipcfg.go#L151

That being said, if this new version has already been tested thoroughly, there is probably no string reason to change it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antoninbas , we used to think about the solution as what Wiregurad is taking, a concern is some fields in MibIPForwardRow are using type []byte, which still exists the risk with a shadow copy if the memory resource is limited (e.g., Windows OS re-allocate the memory to other objects after the MibIPForwardTable is free). So we finally choose the solution to deep copy the objects before free the memory. The latest code is verified on the problematic cluster.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean [N]byte types like the [26]byte field in RawSockAddrInet ([]byte would not be possible here AFAIK)? These should be no different than integer types, they should be inlined in the struct with no indirection.

The code before this change was wrong for obvious reasons. I don't think the Wireguard code is wrong. But if you feel like manual copy is the best approach and this has been well-tested, as I wrote before we can merge it as is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fields in MibIPForwardRow is array not slice. It will be deepcopied automatically.
And the current code is inconsistent: it makes a manual deepcopy for RawSockAddrInet but not AddressPrefix which also has a RawSockAddrInet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example: https://play.golang.com/p/z6Gc0YhEQSV

rows := make([]MibIPForwardRow, table.NumEntries, table.NumEntries)

pFirstRow := uintptr(unsafe.Pointer(&table.Table[0]))
rowSize := unsafe.Sizeof(table.Table[0])

for i := uint32(0); i < table.NumEntries; i++ {
row := *(*MibIPForwardRow)(unsafe.Pointer(pFirstRow + rowSize*uintptr(i)))
rows[i] = row
}
return rows, nil
Comment on lines -371 to +380
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this fix a specific issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We received a report indicating that when host memory is less than 8GB, using unsafe.Slice can lead to memory faults. It’s possible that this function didn’t perform a deep copy, so after calling freeMibTable, we may encounter invalid memory addresses when access rows again. We have already verified that this deep copy method resolves this issue in the same environment.

}

func NewIPForwardRow() *MibIPForwardRow {
Expand Down
Loading