Fix invalid conversion between unsafe.Pointer and uintptr by tnqn · Pull Request #6673 · antrea-io/antrea

tnqn · 2024-09-14T10:12:45Z

uintptr cannot be stored in variable before conversion back to Pointer. This is because the variable is an integer with no pointer semantics, even if a uintptr holds the address of some object, the garbage collector will not update that uintptr's value if the object moves, nor will that uintptr keep the object from being reclaimed.

The issue can be detected by checkptr when running unit test with race detector:

checkptr: pointer arithmetic result points to invalid allocation

pFirstRow := uintptr(unsafe.Pointer(&table.Table[0]))
row := *(*MibIPForwardRow)(unsafe.Pointer(pFirstRow + rowSize*uintptr(i)))

While it can be fixed by performing both conversions in the same expression like below, using unsafe.Slice to get the slice is much simpler.

row := *(*MibIPForwardRow)(unsafe.Pointer(uintptr(unsafe.Pointer(&table.Table[0])) + rowSize*uintptr(i)))

The patch also adds an unit test to validate it.

tnqn · 2024-09-14T15:29:34Z

+	syscallN := func(trap uintptr, args ...uintptr) (r1, r2 uintptr, err syscall.Errno) {
+		// This mocks the 1st call to syscallN made by getIPForwardTable. It makes args[1] point to the table.
+		if len(args) > 1 {
+			ptr := unsafe.Pointer(args[1])


Somehow checkptr thinks "pointer arithmetic result points to invalid allocation".
There is no problem when -race is not used. Need more investigation.

antoninbas · 2024-09-16T17:45:27Z

 type netIO struct {
-	syscallN func(trap uintptr, args ...uintptr) (r1, r2 uintptr, err syscall.Errno)
+	syscallN            func(trap uintptr, args ...uintptr) (r1, r2 uintptr, err syscall.Errno)
+	getIPForwardTableFn func(family uint16, ipForwardTable **MibIPForwardTable) (errcode error)


couldn't we just fake syscallN in tests, instead of faking individual functions like getIPForwardTable? It seems that getIPForwardTable is just a thin wrapper around a syscallN call.

That's what I did in an earlier version: b2e1f7a. However, checkptr failed when --race is enabled, as I commented in #6673 (comment). The error can be found here: https://github.com/antrea-io/antrea/actions/runs/10863106542/job/30146883490.

I suspect the compiler doesn't recognize the fake syscall and mistakenly raise the error. But even the latest version failed due to another checkptr error in code (as opposed the previous error in test code), and this error seems valid: the code shouldn't assign uintptr to a variable.

tnqn · 2024-10-12T17:00:05Z

/test-windows-all

luolanzone · 2024-10-14T03:33:57Z

If the previous way will lead to invalid conversion, I suppose we need this PR and #6664 for the final solution to fix Windows memory issue? @wenyingd If so, I think we need to cherry-pick this in downstream first before Antrea 2.2 is released.

XinShuYang · 2024-10-14T04:34:37Z

 		return nil, os.NewSyscallError("iphlpapi.GetIpForwardTable", err)
 	}
-	rows := make([]MibIPForwardRow, table.NumEntries, table.NumEntries)
+	defer n.freeMibTable(unsafe.Pointer(table))


The reason for removing “if table != nil” is that when err != nil, the memory release logic has already been correctly handled, so it won't cause a memory leak, right?

when err is not nil, the syscall failed and there is no memory to release, isn't it?

It should be, but it makes the original implementation a bit confusing. I don't understand why "if table != nil" check is needed before "if err != nil"(I recall that this part was copied from another project). @wenyingd Do you know where the source code is, and is there's any potential risk? Based on the function doc I found at https://learn.microsoft.com/en-us/windows/win32/api/iphlpapi/nf-iphlpapi-getipforwardtable, the current code change is safe.

https://github.com/tailscale/winipcfg-go/blob/main/wt_mib_ipforward_row2.go#L58
This should be the place where we originally referred.
It should be good with the version in this change.

The code is safe unless the syscall returns a table and an error at the same time, which doesn't seem normal in golang. And wireguard doesn't try to free the memory either when there is an error: https://github.com/WireGuard/wireguard-windows/blob/e70799b1440690e7d4140bffc7c73baf903c7b54/tunnel/winipcfg/winipcfg.go#L145

antoninbas

LGTM

antoninbas · 2024-10-14T17:13:56Z

+		},
+	}
+	gotRows, gotErr := testNetIO.ListIPForwardRows(AF_INET)
+	require.Nil(t, gotErr)


nit: require.NoError

antoninbas · 2024-10-14T17:15:28Z

+	assert.True(t, freeMibTableCalled)
+	// It verifies that the returned rows are independent copies, not referencing to the original table's memory, by
+	// asserting they retain the exact same content as the original table whose rows have been reset by freeMibTable.
+	expectedRows := []MibIPForwardRow{


it feels like we could declare row1 and row2 variables, to avoid repeating the row definitions in the test

uintptr cannot be stored in variable before conversion back to Pointer. This is because the variable is an integer with no pointer semantics, even if a uintptr holds the address of some object, the garbage collector will not update that uintptr's value if the object moves, nor will that uintptr keep the object from being reclaimed. The issue can be detected by checkptr when running unit test with race detector: checkptr: pointer arithmetic result points to invalid allocation ``` pFirstRow := uintptr(unsafe.Pointer(&table.Table[0])) row := *(*MibIPForwardRow)(unsafe.Pointer(pFirstRow + rowSize*uintptr(i))) ``` While it can be fixed by performing both conversions in the same expression like below, using `unsafe.Slice` to get the slice is much simpler. ``` row := *(*MibIPForwardRow)(unsafe.Pointer(uintptr(unsafe.Pointer(&table.Table[0])) + rowSize*uintptr(i))) ``` The patch also adds an unit test to validate it. Signed-off-by: Quan Tian <quan.tian@broadcom.com>

tnqn · 2024-10-15T10:24:49Z

@wenyingd @XinShuYang will the windows e2e tests cover the updated code?

wenyingd · 2024-10-15T10:31:10Z

@wenyingd @XinShuYang will the windows e2e tests cover the updated code?

The OSS e2e test cases can cover the function logic of the updated code. But the original failure is hit in scale setup, OSS e2e doesn't cover scale test.

tnqn · 2024-10-15T13:29:32Z

The OSS e2e test cases can cover the function logic of the updated code. But the original failure is hit in scale setup, OSS e2e doesn't cover scale test.

OK, it's fine as long as it covers the function logic. The original failure was due to misuse of memory, which has been covered by the new unit test. The unit test failed with the original code: https://github.com/tnqn/antrea/actions/runs/11344284459/job/31549023056?pr=46.

antoninbas

LGTM

XinShuYang · 2024-10-16T03:00:14Z

/test-windows-all

tnqn · 2024-10-16T07:23:39Z

/test-windows-all
/skip-all

tnqn · 2024-10-16T14:43:42Z

/test-windows-all

…6673) uintptr cannot be stored in variable before conversion back to Pointer. This is because the variable is an integer with no pointer semantics, even if a uintptr holds the address of some object, the garbage collector will not update that uintptr's value if the object moves, nor will that uintptr keep the object from being reclaimed. The issue can be detected by checkptr when running unit test with race detector: checkptr: pointer arithmetic result points to invalid allocation ``` pFirstRow := uintptr(unsafe.Pointer(&table.Table[0])) row := *(*MibIPForwardRow)(unsafe.Pointer(pFirstRow + rowSize*uintptr(i))) ``` While it can be fixed by performing both conversions in the same expression like below, using `unsafe.Slice` to get the slice is much simpler. ``` row := *(*MibIPForwardRow)(unsafe.Pointer(uintptr(unsafe.Pointer(&table.Table[0])) + rowSize*uintptr(i))) ``` The patch also adds an unit test to validate it. Signed-off-by: Quan Tian <quan.tian@broadcom.com>

uintptr cannot be stored in variable before conversion back to Pointer. This is because the variable is an integer with no pointer semantics, even if a uintptr holds the address of some object, the garbage collector will not update that uintptr's value if the object moves, nor will that uintptr keep the object from being reclaimed. The issue can be detected by checkptr when running unit test with race detector: checkptr: pointer arithmetic result points to invalid allocation ``` pFirstRow := uintptr(unsafe.Pointer(&table.Table[0])) row := *(*MibIPForwardRow)(unsafe.Pointer(pFirstRow + rowSize*uintptr(i))) ``` While it can be fixed by performing both conversions in the same expression like below, using `unsafe.Slice` to get the slice is much simpler. ``` row := *(*MibIPForwardRow)(unsafe.Pointer(uintptr(unsafe.Pointer(&table.Table[0])) + rowSize*uintptr(i))) ``` The patch also adds an unit test to validate it. Signed-off-by: Quan Tian <quan.tian@broadcom.com>

…6673) uintptr cannot be stored in variable before conversion back to Pointer. This is because the variable is an integer with no pointer semantics, even if a uintptr holds the address of some object, the garbage collector will not update that uintptr's value if the object moves, nor will that uintptr keep the object from being reclaimed. The issue can be detected by checkptr when running unit test with race detector: checkptr: pointer arithmetic result points to invalid allocation ``` pFirstRow := uintptr(unsafe.Pointer(&table.Table[0])) row := *(*MibIPForwardRow)(unsafe.Pointer(pFirstRow + rowSize*uintptr(i))) ``` While it can be fixed by performing both conversions in the same expression like below, using `unsafe.Slice` to get the slice is much simpler. ``` row := *(*MibIPForwardRow)(unsafe.Pointer(uintptr(unsafe.Pointer(&table.Table[0])) + rowSize*uintptr(i))) ``` The patch also adds an unit test to validate it. Signed-off-by: Quan Tian <quan.tian@broadcom.com>

tnqn added area/test Issues or PRs related to unit and integration tests. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Sep 14, 2024

tnqn requested review from XinShuYang, antoninbas, luolanzone and wenyingd September 14, 2024 10:12

tnqn force-pushed the unit-test-copy-rows branch 3 times, most recently from a30bd29 to b2e1f7a Compare September 14, 2024 14:17

tnqn commented Sep 14, 2024

View reviewed changes

tnqn force-pushed the unit-test-copy-rows branch 3 times, most recently from 7ed92a0 to 10983b1 Compare September 14, 2024 16:24

antoninbas reviewed Sep 16, 2024

View reviewed changes

tnqn marked this pull request as draft September 19, 2024 02:53

tnqn mentioned this pull request Oct 9, 2024

Automated cherry pick of #6664: Replace unsafe.Slice with memory copying to avoid potential #6670

Merged

tnqn force-pushed the unit-test-copy-rows branch 3 times, most recently from 86e1eb8 to db964e2 Compare October 12, 2024 16:43

tnqn changed the title ~~Add unit test for ListIPForwardRows~~ Fix invalid conversion between unsafe.Pointer and uintptr Oct 12, 2024

tnqn added action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. area/OS/windows Issues or PRs related to the Windows operating system. labels Oct 12, 2024

tnqn marked this pull request as ready for review October 12, 2024 16:47

tnqn removed area/test Issues or PRs related to unit and integration tests. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Oct 14, 2024

XinShuYang reviewed Oct 14, 2024

View reviewed changes

antoninbas previously approved these changes Oct 14, 2024

View reviewed changes

tnqn dismissed antoninbas’s stale review via 9396532 October 15, 2024 10:22

tnqn force-pushed the unit-test-copy-rows branch from db964e2 to 9396532 Compare October 15, 2024 10:22

antoninbas approved these changes Oct 15, 2024

View reviewed changes

tnqn merged commit aad06cd into antrea-io:main Oct 17, 2024

tnqn deleted the unit-test-copy-rows branch October 17, 2024 07:27

Conversation

tnqn commented Sep 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnqn Sep 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnqn commented Oct 12, 2024

Uh oh!

luolanzone commented Oct 14, 2024

Uh oh!

XinShuYang Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antoninbas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnqn commented Oct 15, 2024

Uh oh!

wenyingd commented Oct 15, 2024

Uh oh!

tnqn commented Oct 15, 2024

Uh oh!

antoninbas left a comment

Choose a reason for hiding this comment

Uh oh!

XinShuYang commented Oct 16, 2024

Uh oh!

tnqn commented Oct 16, 2024

Uh oh!

tnqn commented Oct 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tnqn commented Sep 14, 2024 •

edited

Loading

tnqn Sep 14, 2024 •

edited

Loading

XinShuYang Oct 14, 2024 •

edited

Loading