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

runtime: pointer to struct field can point beyond struct allocation #9401

Closed
rsc opened this issue Dec 19, 2014 · 9 comments

Comments

@rsc
Copy link
Contributor

commented Dec 19, 2014

If a non-zero-size struct contains a final zero-size field f, the address &x.f may point beyond the allocation for the struct. This could cause a memory leak or a crash in the garbage collector (invalid pointer found). This is a more general version of #9384.

@rsc rsc added this to the Go1.5 milestone Dec 19, 2014

@dvyukov

This comment has been minimized.

Copy link
Member

commented Dec 20, 2014

We can move all zero-size fields to the beginning of structs.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2014

That would work for any user-defined structs. But we could just as easily convert &(zero-sized thing) to a pointer to the zero object in the compiler.

I'm a bit more concerned about generically built structs, like reflect.call and reflect.funcLayout.

@rsc

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2014

It seems like there are two options.

  1. Fix the code manipulating the struct memory never to point past the end.
  2. Add more memory to the struct so that all the existing manipulation code becomes correct.

I believe that since zero-size types are excluded from the problem, we've narrowed the problem to very rare cases. The best solution is probably the one that introduces the fewest new corner cases where bugs can hide. There are many fewer places that decide struct layout and allocation than there are things that walk structs, which means we should favor choice 2. (Another point in favor of choice 2: choice 1 may require changes to code we don't even control; what if something like goprotobuf broke these rules?)

In #9384, Keith proposed a new type flag to indicate that the memory footprint ends in a zero-length field, so that allocation would add just a little more room after that. That would probably work, but given the previous argument, I think that's more subtle than this problem warrants. I want something that's dead simple. I worry that if new(T) sometimes (but almost never) allocates > T.size bytes, that will create a space for bugs.

I propose the dumber solution that if a non-zero-sized struct ends in a zero-length field, we add 1 byte of padding to end of the struct before aligning the final struct size. That is, given:

type T1 struct {
    X byte
    Y [0]byte
}

type T2 struct {
    X uint32
    Y struct{}
}

T1 would have size 2 (= 1 + 0 + 1) and T2 would have size 8 (= 4 + 0 + 1 + padding to 4-byte alignment for field X).

In cases where this padding is a problem, it can be avoided by manual rearrangement or removal of the zero-length fields, as Keith did in the fix for #9384.

I think all the things making structs at runtime have to build gcProgs, and there are only two mentions of gcProg in package reflect: bucketOf and funcLayout. bucketOf is already fixed. funcLayout could just add regSize to the total size any time the total size is not 0. This would also make safe the use of frame+retOffset when functions have arguments but no results.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Dec 23, 2014

That sounds like a reasonable plan to me.
I curious about whether we need to do anything for chan struct{}. I don't
think we actually materialize a pointer to the struct elements in this
case, but I'm not sure. I think we set the elem pointer to point back to
the channel header.

On Mon, Dec 22, 2014 at 10:06 PM, Russ Cox notifications@github.com wrote:

It seems like there are two options.

  1. Fix the code manipulating the struct memory never to point past the
    end.
  2. Add more memory to the struct so that all the existing manipulation
    code becomes correct.

I believe that since zero-size types are excluded from the problem, we've
narrowed the problem to very rare cases. The best solution is probably the
one that introduces the fewest new corner cases where bugs can hide. There
are many fewer places that decide struct layout and allocation than there
are things that walk structs, which means we should favor choice 2.
(Another point in favor of choice 2: choice 1 may require changes to code
we don't even control; what if something like goprotobuf broke these rules?)

In #9384 #9384, Keith proposed a new
type flag to indicate that the memory footprint ends in a zero-length
field, so that allocation would add just a little more room after that.
That would probably work, but given the previous argument, I think that's
more subtle than this problem warrants. I want something that's dead
simple. I worry that if new(T) sometimes (but almost never) allocates >
T.size bytes, that will create a space for bugs.

I propose the dumber solution that if a non-zero-sized struct ends in a
zero-length field, we add 1 byte of padding to end of the struct before
aligning the final struct size. That is, given:

type T1 struct {
X byte
Y [0]byte
}

type T2 struct {
X uint32
Y struct{}
}

T1 would have size 2 (= 1 + 0 + 1) and T2 would have size 8 (= 4 + 0 + 1 +
padding to 4-byte alignment for field X).

In cases where this padding is a problem, it can be avoided by manual
rearrangement or removal of the zero-length fields, as Keith did in the fix
for #9384 #9384.

I think all the things making structs at runtime have to build gcProgs,
and there are only two mentions of gcProg in package reflect: bucketOf and
funcLayout. bucketOf is already fixed. funcLayout could just add regSize to
the total size any time the total size is not 0. This would also make safe
the use of frame+retOffset when functions have arguments but no results.


Reply to this email directly or view it on GitHub
#9401 (comment).

@randall77

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2015

This looks a bit tricky. We have several structs like the following in the runtime:

type interfacetype struct {
typ _type
mhdr []imethod
m [0]imethod
}

The [0] is really a placeholder for a variable-length array to follow.

Then to get the address of the nth imethod in interfacetype i, we do:
add(unsafe.Pointer(i), unsafe.Sizeof(interfacetype{}) + n * unsafe.Sizeof(imethod{}))

That math is all wrong if we add a byte and then round the size of interfacetype.
(Why don't we use &i.mhdr[n] instead of that math? I'm not sure. Maybe just to avoid the bounds check? Could do &i.m + n * ... also.)

Other structs which use the [0] at the end trick include m, g, stackmap, itab, and uncommontype.

It is possible that users depend on this trick as well.
I'm thinking the add-a-byte at malloc time for objects with trailing zero fields is the safer way to go. We already round up to size classes so I don't think any code would be surprised by the allocated size being slightly larger than the requested size.

gopherbot pushed a commit that referenced this issue Jan 7, 2015

runtime: remove trailing empty arrays in structs
The ones at the end of M and G are just used to compute
their size for use in assembly.  Generate the size explicitly.
The one at the end of itab is variable-sized, and at least one.
The ones at the end of interfacetype and uncommontype are not
needed, as the preceding slice references them (the slice was
originally added for use by reflect?).
The one at the end of stackmap is already accessed correctly,
and the runtime never allocates one.

Update #9401

Change-Id: Ia75e3aaee38425f038c506868a17105bd64c712f
Reviewed-on: https://go-review.googlesource.com/2420
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>

gopherbot pushed a commit that referenced this issue Jan 7, 2015

runtime: add comment about channels already handling zero-sized objec…
…ts correctly.

update #9401

Change-Id: I634a772814e7cd066f631a68342e7c3dc9d27e72
Reviewed-on: https://go-review.googlesource.com/2370
Reviewed-by: Russ Cox <rsc@golang.org>

@gopherbot gopherbot closed this in 6f07ac2 Jan 8, 2015

@gopherbot

This comment has been minimized.

Copy link

commented Jul 30, 2015

CL https://golang.org/cl/12864 mentions this issue.

ianlancetaylor added a commit that referenced this issue Jul 30, 2015

cmd/cgo: discard trailing zero-sized fields in a non-empty C struct
In order to fix issue #9401 the compiler was changed to add a padding
byte to any non-empty Go struct that ends in a zero-sized field.  That
causes the Go version of such a C struct to have a different size than
the C struct, which can considerable confusion.  Change cgo so that it
discards any such zero-sized fields, so that the Go and C structs are
the same size.

This is a change from previous releases, in that it used to be
possible to refer to a zero-sized trailing field (by taking its
address), and with this change it no longer is.  That is unfortunate,
but something has to change.  It seems better to visibly break
programs that do this rather than to silently break programs that rely
on the struct sizes being the same.

Update #9401.
Fixes #11925.

Change-Id: I3fba3f02f11265b3c41d68616f79dedb05b81225
Reviewed-on: https://go-review.googlesource.com/12864
Reviewed-by: Russ Cox <rsc@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Oct 9, 2015

CL https://golang.org/cl/15660 mentions this issue.

minux added a commit that referenced this issue Oct 13, 2015

cmd/internal/obj: move empty field to the top
Due to #9401, trailing empty fields will occupy at least 1 byte
of space.

Fixes #12884.

Change-Id: I838d3f1a73637e526f5a6dbc348981227d5bb2fd
Reviewed-on: https://go-review.googlesource.com/15660
Run-TryBot: Minux Ma <minux@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dave Cheney <dave@cheney.net>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Jan 12, 2016

CL https://golang.org/cl/18542 mentions this issue.

gopherbot pushed a commit that referenced this issue Jan 12, 2016

cmd/cgo: document C.sizeof_T and zero-sized field restriction
Update #9401.
Update #11925.
Update #13919.

Change-Id: I52c679353693e8165b2972d4d3974ee8bb1207ef
Reviewed-on: https://go-review.googlesource.com/18542
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Nov 22, 2016

CL https://golang.org/cl/33475 mentions this issue.

gopherbot pushed a commit that referenced this issue Nov 23, 2016

reflect: fix size of StructOf ending in zero-sized field
Update #9401.
Fixes #18016.

Change-Id: Icc24dd10dab1ad8e5cf295e0727d437afa5025c0
Reviewed-on: https://go-review.googlesource.com/33475
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

@golang golang locked and limited conversation to collaborators Nov 23, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.