[go] runtime: pass actual allocation size to special free
Daniel Morsing (Gerrit)
Daniel Morsing has uploaded the change for review![]( "Open in Gerrit")
Daniel Morsing would like Michael Knyszek to review this change.
Commit message
runtime: pass actual allocation size to special free
During a naive attempt to test the new runtime/secret package, I tried
wrapping the entire handshake in a secret.Do call. This lead to a panic
because some of the allocator logic had been previously untested.
The problem was that the sweep code passed an adjusted pointer to
freeSpecial, which points beyond the type header, but still had the size
of the span element. When the runtime/secret code then zeroed the
allocation, it would overwrite the type header of the span element one
over.
Fix by adjusting the size when we adjust the pointer to pass to
freeSpecial
Change diff
diff --git a/src/runtime/mgcsweep.go b/src/runtime/mgcsweep.go
index 4eecb1c..143816f 100644
--- a/src/runtime/mgcsweep.go
+++ b/src/runtime/mgcsweep.go
@@ -26,6 +26,7 @@
import (
"internal/runtime/atomic"
+ "internal/runtime/gc"
"unsafe"
)
@@ -532,7 +533,16 @@
mheap_.pagesSwept.Add(int64(s.npages))
spc := s.spanclass
- size := s.elemsize
+ // elemSize is the size of the element in the span.
+ // allocSize is the size of the allocation itself.
+ // The difference is important to freespecial, since
+ // it only wants to work on the actual allocation
+ elemSize := s.elemsize
+ allocSize := elemSize
+ // element has a header, exclude it from the size
+ if spc.sizeclass() != 0 {
+ allocSize -= gc.MallocHeaderSize
+ }
// The allocBits indicate which unmarked objects don't need to be
// processed since they were free at the end of the last GC cycle
@@ -554,14 +564,14 @@
siter := newSpecialsIter(s)
for siter.valid() {
// A finalizer can be set for an inner byte of an object, find object beginning.
- objIndex := siter.s.offset / size
- p := s.base() + objIndex*size
+ objIndex := siter.s.offset / elemSize
+ p := s.base() + objIndex*elemSize
mbits := s.markBitsForIndex(objIndex)
if !mbits.isMarked() {
// This object is not marked and has at least one special record.
// Pass 1: see if it has a finalizer.
hasFinAndRevived := false
- endOffset := p - s.base() + size
+ endOffset := p - s.base() + elemSize
for tmp := siter.s; tmp != nil && tmp.offset < endOffset; tmp = tmp.next {
if tmp.kind == _KindSpecialFinalizer {
// Stop freeing of object if it has a finalizer.
@@ -581,7 +591,7 @@
p := s.base() + special.offset
if special.kind == _KindSpecialFinalizer || special.kind == _KindSpecialWeakHandle {
siter.unlinkAndNext()
- freeSpecial(special, unsafe.Pointer(p), size)
+ freeSpecial(special, unsafe.Pointer(p), allocSize)
} else {
// All other specials only apply when an object is freed,
// so just keep the special record.
@@ -596,7 +606,7 @@
special := siter.s
p := s.base() + special.offset
siter.unlinkAndNext()
- freeSpecial(special, unsafe.Pointer(p), size)
+ freeSpecial(special, unsafe.Pointer(p), allocSize)
}
}
} else {
@@ -604,7 +614,7 @@
if siter.s.kind == _KindSpecialReachable {
special := siter.unlinkAndNext()
(*specialReachable)(unsafe.Pointer(special)).reachable = true
- freeSpecial(special, unsafe.Pointer(p), size)
+ freeSpecial(special, unsafe.Pointer(p), allocSize)
} else {
// keep special record
siter.next()
@@ -630,17 +640,17 @@
}
}
if debug.clobberfree != 0 {
- clobberfree(unsafe.Pointer(x), size)
+ clobberfree(unsafe.Pointer(x), elemSize)
}
// User arenas are handled on explicit free.
if raceenabled && !s.isUserArenaChunk {
- racefree(unsafe.Pointer(x), size)
+ racefree(unsafe.Pointer(x), elemSize)
}
if msanenabled && !s.isUserArenaChunk {
- msanfree(unsafe.Pointer(x), size)
+ msanfree(unsafe.Pointer(x), elemSize)
}
if asanenabled && !s.isUserArenaChunk {
- asanpoison(unsafe.Pointer(x), size)
+ asanpoison(unsafe.Pointer(x), elemSize)
}
if valgrindenabled && !s.isUserArenaChunk {
valgrindFree(unsafe.Pointer(x))
@@ -807,11 +817,11 @@
// from inHeap might overflow. See #67019.
stats := memstats.heapStats.acquire()
atomic.Xadd64(&stats.largeFreeCount, 1)
- atomic.Xadd64(&stats.largeFree, int64(size))
+ atomic.Xadd64(&stats.largeFree, int64(elemSize))
memstats.heapStats.release()
// Count the free in the inconsistent, internal stats.
- gcController.totalFree.Add(int64(size))
+ gcController.totalFree.Add(int64(elemSize))
// NOTE(rsc,dvyukov): The original implementation of efence
// in CL 22060046 used sysFree instead of sysFault, so that
@@ -829,7 +839,7 @@
// implement and then call some kind of mheap.deleteSpan.
if debug.efence > 0 {
s.limit = 0 // prevent mlookup from finding this span
- sysFault(unsafe.Pointer(s.base()), size)
+ sysFault(unsafe.Pointer(s.base()), elemSize)
} else {
mheap_.freeSpan(s)
}
Change information
- M src/runtime/mgcsweep.go
Related details
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 1 comment![]( "Open in Gerrit")
Given that I ran into this myself, should I create an issue for this?
Also, I'm unsure how to test. Right now I'm verifying that the problem doesn't occur by running the TLS handshake benchmarks with a patch that wraps the handshake in a secret.Do block. My attempts so far at a more limited reproducer has not been fruitful.
Related details
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds added 2 comments![]( "Open in Gerrit")
Hi @daniel....@gmail.com, a couple of quick drive by comments. (I'm just an external contributor, so please just consider all of this as "food for thought").
Are you able to test this? It might be good to have tests for noscan and scan objects, maybe using a user-facing special creation API like `runtime.AddCleanup` or similar, with a few different sizes to help get coverage of the different size-based shifts in mallocgc behavior (e.g., something less than 512 bytes, something > 512 bytes but less than 32KiB-8, and something > 32KiB). Or maybe those specifics are not appropriate based on the bug at hand, but it seems like this should be testable.
Also, it might be worth considering adding something like an off-by-default `doubleCheck` constant somewhere so that when it is enabled, you then use something like `findObject` (which returns an object's base) to sanity check that you point to the base of the object after all your pointer adjustments are complete (and throw otherwise), or something along those lines. That might make it easier to reproduce before the fix with the constant temporarily enabled, and then give some extra comfort with the fix in place. If you grep for `doubleCheck` in the runtime directory, you can see some prior art in malloc.go and mbitmap.go and the like.
`findObject`:
https://github.com/golang/go/blob/master/src/runtime/mbitmap.go#L1337-L1361
Finally, sorry if this is all things you already know. 😊
allocSize -= gc.MallocHeaderSizeIs this the right check for whether something has a malloc header?
Off the cuff, I guess I would have thought it would need to check noscan and the return value of heapBitsInSpan(size), or something like that. I would have to poke around to make a better guess of the right way, but the current check does not seem obviously correct based on my (possibly incorrect) knowledge.
Or if that is in fact the right check, maybe the comment could briefly explain why.
Related details
- Daniel Morsing
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Knyszek (Gerrit)
Michael Knyszek added 2 comments![]( "Open in Gerrit")
freeSpecial, which points beyond the type header, but still had the size
of the span element. When the runtime/secret code then zeroed theit's tempting to think that freeSpecial accepts a [p, p+size) address range, but it in fact does not. (this is a fact I had missed in the review as well.)
there are multiple cases (besides secrets) where p and size don't make sense together, because p is derived from the offset field of the special. for example, for cleanups and weak pointers, p can be any arbitrary offset inside the allocation.
this change as-is also changes the semantics of the heap profile which is a little unfortunate (we count the type header as part of the allocation currently).
here's my suggestion:
1. rename p and size to make it clear that these do NOT represent [p, p+size),
2. move the adjustment logic into the freeSpecial part for secrets OR into addSecret.
I think a clean way to do both of these is to pass the `*mspan` instead of a size. it's just using `elemsize` anyway.
then, inside `freeSpecial`, we can do the correct adjustment for the malloc headers.
or, even simpler alternative than doing the adjustment for secrets specifically could be to *always* make the special's offset exactly zero in `addSecret`, meaning it would include the type header, which would make the assumption of [p, p+size) true for at least secrets. (I think we should make this change in addition to the ones I proposed above, to prevent mistakes like this in the future.)
allocSize -= gc.MallocHeaderSizeIs this the right check for whether something has a malloc header?
Off the cuff, I guess I would have thought it would need to check noscan and the return value of heapBitsInSpan(size), or something like that. I would have to poke around to make a better guess of the right way, but the current check does not seem obviously correct based on my (possibly incorrect) knowledge.
Or if that is in fact the right check, maybe the comment could briefly explain why.
this is not the right check, you want spc.sizeclass() != 0 && !noscan && !heapBitsInSpan(size). but see my other comment.
Related details
- Daniel Morsing
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 2 comments![]( "Open in Gerrit")
Hi @daniel....@gmail.com, a couple of quick drive by comments. (I'm just an external contributor, so please just consider all of this as "food for thought").
Are you able to test this? It might be good to have tests for noscan and scan objects, maybe using a user-facing special creation API like `runtime.AddCleanup` or similar, with a few different sizes to help get coverage of the different size-based shifts in mallocgc behavior (e.g., something less than 512 bytes, something > 512 bytes but less than 32KiB-8, and something > 32KiB). Or maybe those specifics are not appropriate based on the bug at hand, but it seems like this should be testable.
Also, it might be worth considering adding something like an off-by-default `doubleCheck` constant somewhere so that when it is enabled, you then use something like `findObject` (which returns an object's base) to sanity check that you point to the base of the object after all your pointer adjustments are complete (and throw otherwise), or something along those lines. That might make it easier to reproduce before the fix with the constant temporarily enabled, and then give some extra comfort with the fix in place. If you grep for `doubleCheck` in the runtime directory, you can see some prior art in malloc.go and mbitmap.go and the like.
`findObject`:
https://github.com/golang/go/blob/master/src/runtime/mbitmap.go#L1337-L1361Finally, sorry if this is all things you already know. 😊
I tried writing a test that exercised this bug, but it relies on a sequence of events that I'm not entirely sure how to reproduce. Currently I've been reproducing with this: https://gist.github.com/DanielMorsing/aeea05db9c4cfd3f9810b45dbd3b2f8f . It fails almost instantly.
allocSize -= gc.MallocHeaderSizeIs this the right check for whether something has a malloc header?
Off the cuff, I guess I would have thought it would need to check noscan and the return value of heapBitsInSpan(size), or something like that. I would have to poke around to make a better guess of the right way, but the current check does not seem obviously correct based on my (possibly incorrect) knowledge.
Or if that is in fact the right check, maybe the comment could briefly explain why.
The ad-hoc test I have right now panicked during `getGCMask` at `typePointersOfUnchecked`. I followed the lead from that code on whether the element has the header, but I can believe that by the time the code gets to that point, some important preconditions have already been checked and branched on.
Related details
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds added 1 comment![]( "Open in Gerrit")
allocSize -= gc.MallocHeaderSizeDaniel MorsingIs this the right check for whether something has a malloc header?
Off the cuff, I guess I would have thought it would need to check noscan and the return value of heapBitsInSpan(size), or something like that. I would have to poke around to make a better guess of the right way, but the current check does not seem obviously correct based on my (possibly incorrect) knowledge.
Or if that is in fact the right check, maybe the comment could briefly explain why.
The ad-hoc test I have right now panicked during `getGCMask` at `typePointersOfUnchecked`. I followed the lead from that code on whether the element has the header, but I can believe that by the time the code gets to that point, some important preconditions have already been checked and branched on.
FWIW, while you are testing, it might be worth doing a sed or similar to temporarily flip all the `const doubleCheck = false` in `mbitmap.go` to `true` (including the one in `typePointersOfUnchecked`), as well as flip `doubleCheckMalloc` temporarily to true.
I recently added a small comment more-or-less saying this:
https://github.com/golang/go/blob/master/src/runtime/malloc.go#L1083-L1085
Some chance that doesn't materially help, but it's an easy temporary change and might make it much easier to reproduce the core problem, or at least it would be some extra sanity checks while you are making changes (e.g., if you have some mid-development typo or bug, it might catch it immediately rather than it being heisenbug-ish). YMMV, but I can attest that those extra checks were very helpful to me while making some malloc and free related changes during 1.26 cycle.
Related details
- Daniel Morsing
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Knyszek (Gerrit)
Michael Knyszek added 1 comment![]( "Open in Gerrit")
allocSize -= gc.MallocHeaderSizeDaniel MorsingIs this the right check for whether something has a malloc header?
Off the cuff, I guess I would have thought it would need to check noscan and the return value of heapBitsInSpan(size), or something like that. I would have to poke around to make a better guess of the right way, but the current check does not seem obviously correct based on my (possibly incorrect) knowledge.
Or if that is in fact the right check, maybe the comment could briefly explain why.
t hepuddsThe ad-hoc test I have right now panicked during `getGCMask` at `typePointersOfUnchecked`. I followed the lead from that code on whether the element has the header, but I can believe that by the time the code gets to that point, some important preconditions have already been checked and branched on.
FWIW, while you are testing, it might be worth doing a sed or similar to temporarily flip all the `const doubleCheck = false` in `mbitmap.go` to `true` (including the one in `typePointersOfUnchecked`), as well as flip `doubleCheckMalloc` temporarily to true.
I recently added a small comment more-or-less saying this:
https://github.com/golang/go/blob/master/src/runtime/malloc.go#L1083-L1085Some chance that doesn't materially help, but it's an easy temporary change and might make it much easier to reproduce the core problem, or at least it would be some extra sanity checks while you are making changes (e.g., if you have some mid-development typo or bug, it might catch it immediately rather than it being heisenbug-ish). YMMV, but I can attest that those extra checks were very helpful to me while making some malloc and free related changes during 1.26 cycle.
yeah, `typePointersOfUnchecked` definitely "distributes" the preconditions because it handles all the different cases separately. if you want to just check if there's a malloc header on allocations in a span, that's the condition (not large (>32 KiB), has pointers, and !heapBitsInSpan (which is a "not small" check)).
it's not a pretty condition and we haven't had a need to actually check it completely anywhere, so there's no clean function that just tells you.
Related details
- Daniel Morsing
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds added 1 comment![]( "Open in Gerrit")
Daniel MorsingHi @daniel....@gmail.com, a couple of quick drive by comments. (I'm just an external contributor, so please just consider all of this as "food for thought").
Are you able to test this? It might be good to have tests for noscan and scan objects, maybe using a user-facing special creation API like `runtime.AddCleanup` or similar, with a few different sizes to help get coverage of the different size-based shifts in mallocgc behavior (e.g., something less than 512 bytes, something > 512 bytes but less than 32KiB-8, and something > 32KiB). Or maybe those specifics are not appropriate based on the bug at hand, but it seems like this should be testable.
Also, it might be worth considering adding something like an off-by-default `doubleCheck` constant somewhere so that when it is enabled, you then use something like `findObject` (which returns an object's base) to sanity check that you point to the base of the object after all your pointer adjustments are complete (and throw otherwise), or something along those lines. That might make it easier to reproduce before the fix with the constant temporarily enabled, and then give some extra comfort with the fix in place. If you grep for `doubleCheck` in the runtime directory, you can see some prior art in malloc.go and mbitmap.go and the like.
`findObject`:
https://github.com/golang/go/blob/master/src/runtime/mbitmap.go#L1337-L1361Finally, sorry if this is all things you already know. 😊
I tried writing a test that exercised this bug, but it relies on a sequence of events that I'm not entirely sure how to reproduce. Currently I've been reproducing with this: https://gist.github.com/DanielMorsing/aeea05db9c4cfd3f9810b45dbd3b2f8f . It fails almost instantly.
Here's what might be be a simple reproducer. Seems to reproduce reliably on the playground:
https://go.dev/play/p/8rMXfAdtpyI?v=gotip
In short, it allocates heap objects with type headers where it purposefully interleaves heap objects that are kept alive beyond secret.Do with heap objects that are not, with the intent of tickling a bug where clearing one heap object with the wrong size wipes out the type pointer for the next allocated slot.
Currently fails on tip with:
```
SIGSEGV: segmentation violation
PC=0x42a47d m=3 sigcode=1 addr=0x14
goroutine 0 gp=0x395fe62def00 m=3 mp=0x395fe6329008 [idle]:
runtime.getGCMask(...)
/usr/local/go-faketime/src/runtime/type.go:88
runtime.(*mspan).typePointersOfUnchecked(0x395fe633fe90?, 0x4172d5?)
/usr/local/go-faketime/src/runtime/mbitmap.go:166 +0x3d fp=0x395fe633fe70 sp=0x395fe633fe40 pc=0x42a47d
runtime.scanObject(0x395fe6305260?, 0x395fe6305260)
/usr/local/go-faketime/src/runtime/mgcmark_greenteagc.go:1235 +0xc5 fp=0x395fe633ff08 sp=0x395fe633fe70 pc=0x43a525
runtime.gcDrain(0x395fe6305260, 0x3)
[...]
```
Related details
- Daniel Morsing
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Daniel Morsing
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Daniel Morsing
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 3 comments![]( "Open in Gerrit")
Daniel MorsingHi @daniel....@gmail.com, a couple of quick drive by comments. (I'm just an external contributor, so please just consider all of this as "food for thought").
Are you able to test this? It might be good to have tests for noscan and scan objects, maybe using a user-facing special creation API like `runtime.AddCleanup` or similar, with a few different sizes to help get coverage of the different size-based shifts in mallocgc behavior (e.g., something less than 512 bytes, something > 512 bytes but less than 32KiB-8, and something > 32KiB). Or maybe those specifics are not appropriate based on the bug at hand, but it seems like this should be testable.
Also, it might be worth considering adding something like an off-by-default `doubleCheck` constant somewhere so that when it is enabled, you then use something like `findObject` (which returns an object's base) to sanity check that you point to the base of the object after all your pointer adjustments are complete (and throw otherwise), or something along those lines. That might make it easier to reproduce before the fix with the constant temporarily enabled, and then give some extra comfort with the fix in place. If you grep for `doubleCheck` in the runtime directory, you can see some prior art in malloc.go and mbitmap.go and the like.
`findObject`:
https://github.com/golang/go/blob/master/src/runtime/mbitmap.go#L1337-L1361Finally, sorry if this is all things you already know. 😊
t hepuddsI tried writing a test that exercised this bug, but it relies on a sequence of events that I'm not entirely sure how to reproduce. Currently I've been reproducing with this: https://gist.github.com/DanielMorsing/aeea05db9c4cfd3f9810b45dbd3b2f8f . It fails almost instantly.
Here's what might be be a simple reproducer. Seems to reproduce reliably on the playground:
https://go.dev/play/p/8rMXfAdtpyI?v=gotip
In short, it allocates heap objects with type headers where it purposefully interleaves heap objects that are kept alive beyond secret.Do with heap objects that are not, with the intent of tickling a bug where clearing one heap object with the wrong size wipes out the type pointer for the next allocated slot.
Currently fails on tip with:
```
SIGSEGV: segmentation violation
PC=0x42a47d m=3 sigcode=1 addr=0x14goroutine 0 gp=0x395fe62def00 m=3 mp=0x395fe6329008 [idle]:
runtime.getGCMask(...)
/usr/local/go-faketime/src/runtime/type.go:88
runtime.(*mspan).typePointersOfUnchecked(0x395fe633fe90?, 0x4172d5?)
/usr/local/go-faketime/src/runtime/mbitmap.go:166 +0x3d fp=0x395fe633fe70 sp=0x395fe633fe40 pc=0x42a47d
runtime.scanObject(0x395fe6305260?, 0x395fe6305260)
/usr/local/go-faketime/src/runtime/mgcmark_greenteagc.go:1235 +0xc5 fp=0x395fe633ff08 sp=0x395fe633fe70 pc=0x43a525
runtime.gcDrain(0x395fe6305260, 0x3)
[...]
```
Thank you so much, this is going straight into the CL
freeSpecial, which points beyond the type header, but still had the size
of the span element. When the runtime/secret code then zeroed theit's tempting to think that freeSpecial accepts a [p, p+size) address range, but it in fact does not. (this is a fact I had missed in the review as well.)
there are multiple cases (besides secrets) where p and size don't make sense together, because p is derived from the offset field of the special. for example, for cleanups and weak pointers, p can be any arbitrary offset inside the allocation.
this change as-is also changes the semantics of the heap profile which is a little unfortunate (we count the type header as part of the allocation currently).
here's my suggestion:
1. rename p and size to make it clear that these do NOT represent [p, p+size),
2. move the adjustment logic into the freeSpecial part for secrets OR into addSecret.I think a clean way to do both of these is to pass the `*mspan` instead of a size. it's just using `elemsize` anyway.
then, inside `freeSpecial`, we can do the correct adjustment for the malloc headers.
or, even simpler alternative than doing the adjustment for secrets specifically could be to *always* make the special's offset exactly zero in `addSecret`, meaning it would include the type header, which would make the assumption of [p, p+size) true for at least secrets. (I think we should make this change in addition to the ones I proposed above, to prevent mistakes like this in the future.)
Went with a simple "pigeonhole value in the special" solution. Will leave the systematic changes to people who actually understand the intricacies of this code 😊
Related details
- Michael Knyszek
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Knyszek (Gerrit)
Michael Knyszek voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Knyszek
| Code-Review | +2 |
1 comment
freeSpecial, which points beyond the type header, but still had the size
of the span element. When the runtime/secret code then zeroed theDaniel Morsingit's tempting to think that freeSpecial accepts a [p, p+size) address range, but it in fact does not. (this is a fact I had missed in the review as well.)
there are multiple cases (besides secrets) where p and size don't make sense together, because p is derived from the offset field of the special. for example, for cleanups and weak pointers, p can be any arbitrary offset inside the allocation.
this change as-is also changes the semantics of the heap profile which is a little unfortunate (we count the type header as part of the allocation currently).
here's my suggestion:
1. rename p and size to make it clear that these do NOT represent [p, p+size),
2. move the adjustment logic into the freeSpecial part for secrets OR into addSecret.I think a clean way to do both of these is to pass the `*mspan` instead of a size. it's just using `elemsize` anyway.
then, inside `freeSpecial`, we can do the correct adjustment for the malloc headers.
or, even simpler alternative than doing the adjustment for secrets specifically could be to *always* make the special's offset exactly zero in `addSecret`, meaning it would include the type header, which would make the assumption of [p, p+size) true for at least secrets. (I think we should make this change in addition to the ones I proposed above, to prevent mistakes like this in the future.)
Went with a simple "pigeonhole value in the special" solution. Will leave the systematic changes to people who actually understand the intricacies of this code 😊
- Daniel Morsing
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |