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

Jump to bottom

reflect: StructOf field offset calculation may overflow #52740

Closed
lmb opened this issue May 6, 2022 · 3 comments
Closed

reflect: StructOf field offset calculation may overflow #52740

lmb opened this issue May 6, 2022 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.19

Comments

@lmb
Copy link
Contributor

lmb commented May 6, 2022

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

% go version
go version go1.18.1 darwin/arm64

Does this issue reproduce with the latest release?

Yes.

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

Darwin / amd64.

What did you do?

func TestLargeArray(t *testing.T) {
	elem := reflect.TypeOf(uint32(0))
	max := int(^uintptr(0) / elem.Size())
	arr := reflect.ArrayOf(max, elem)

	fields := []reflect.StructField{
		{Name: "A", Type: arr},
		{Name: "B", Type: reflect.TypeOf("")},
	}

	typ := reflect.StructOf(fields)
	var offset uintptr
	for i := 0; i < typ.NumField(); i++ {
		field := typ.Field(i)
		if i > 0 && field.Offset == offset {
			t.Fatalf("field %d: duplicate offset %d", i, field.Offset)
		} else {
			t.Logf("field %d: offset %d", i, field.Offset)
		}
		offset = field.Offset
	}
}

What did you expect to see?

I'd expect offsets to be correct or StructOf to panic.

What did you see instead?

=== RUN   TestLargeArray
    foo_test.go:107: field 0: offset 0
    foo_test.go:105: field 1: duplicate offset 0
--- FAIL: TestLargeArray (0.00s)

I think the problem lies in this call to align:

go/src/reflect/type.go

Line 2630 in f87e28d

offset := align(size, uintptr(ft.align))

On my machine, size is 0xfffffffffffffffc and ft.align is 8 for field B. 0xfffffffffffffffc + 0x7 is 0x10000000000000003 aka 0x0000000000000003 due to truncation / overflow.
@randall77
Copy link
Contributor

Looks like structs aren't defensive against overflow in general. Adding complication, we only store 63 bits of size, not 64.

@randall77 randall77 added this to the Go1.19 milestone May 6, 2022
@randall77 randall77 added the NeedsFix The path to resolution is known, but the work has not been done. label May 6, 2022
@gopherbot
Copy link

Change https://go.dev/cl/412138 mentions this issue: cmd/compile,runtime,reflect: move embedded bit from offset to name

@gopherbot
Copy link

Change https://go.dev/cl/412214 mentions this issue: reflect: when StructOf overflows computing size/offfset, panic

gopherbot pushed a commit that referenced this issue Jun 14, 2022
@randall77
cmd/compile,runtime,reflect: move embedded bit from offset to name
e1e66a0 
Previously we stole a bit from the field offset to encode whether
a struct field was embedded.

Instead, encode that bit in the name field, where we already have
some unused bits to play with. The bit associates naturally with
the name in any case.

This leaves a full uintptr to specify field offsets. This will make
the fix for #52740 cleaner.

Change-Id: I0bfb85564dc26e8c18101bc8b432f332176d7836
Reviewed-on: https://go-review.googlesource.com/c/go/+/412138
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
@golang golang locked and limited conversation to collaborators Jun 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.19
Development

No branches or pull requests
3 participants
@lmb @randall77 @gopherbot