runtime: slice bounds out of range in mheap on darwin/arm64 (iOS 10) #18651
Comments
This is doing a 1 GB allocation and it looks like a few lines earlier in The bad error message is because mheap.sysAlloc is a little confused. Because the darwin/arm64 heap is only 2 GB, we wound up in the 32-bit path. That's not so bad, except that it assumes we can grow the reservation up to 4 GB (_MaxArena32), but mallocinit only reserved auxiliary structures for a 2 GB heap. When we try to grow the spans array past mallocinit's reservation, it crashes. I think this "worked" until 5915ce6 in late October because it silently walked off the end of the spans array and corrupted something else (probably the heap bitmap). In October I changed it to use a slice instead of ugly pointer math, so we now get bounds checking for free where we didn't have any sort of overflow check before. (Also, given the name of the test, shouldn't gob fail gracefully before allocating a huge slice?) |
|
Thanks for the analysis. Somehow I missed this was happening in a test that should be skipped in -short mode. That suggests there's something wrong with my test harness, I believe all.bash should run -short by default. Then there is the issue of making this error clearer (and ideally, handled properly by encoding/gob), but I believe that is a pre-existing condition so we can punt it to 1.9. |
Sounds good. I'm fairly certain the only consequence of not changing this is that you get one panic instead of another, but trying to fix it may have other consequences. :) I've got a fix queued up, which I'll send for Go 1.9. |
|
CL https://golang.org/cl/35253 mentions this issue. |
|
CL https://golang.org/cl/35252 mentions this issue. |
|
/cc @robpike @rsc: Should encoding/gob's TestHugeWriteFails be failing before it attempts to allocate enough memory to encode the entire (huge) input? The runtime bug in this issue was tickled because TestHugeWriteFails attempted to allocate a 1 GB buffer rather than failing early on the huge input. The code is clearly written to encode the whole thing and only then check if it got too big, but I'm wondering if that's what it should be doing. |
|
@aclements If you can write a simple reproducer, please file an issue and assign it to me. |
|
CL https://golang.org/cl/35251 mentions this issue. |
|
@aclements since https://golang.org/cl/35251 removes the broken ulimit -v check, could you link the issue tracking re-adding that check? Even if it's considered not worthy to re-add, people should at least be able to find the reasons. |
|
@nightlyone, thanks. Added a reference to the commit message. |
Currently _MaxMem is a uintptr, which is going to complicate some
further changes. Make it untyped so we'll be able to do untyped math
on it before truncating it to a uintptr.
The runtime assembly is identical before and after this change on
{linux,windows}/{amd64,386}.
Updates #18651.
Change-Id: I0f64511faa9e0aa25179a556ab9f185ebf8c9cf8
Reviewed-on: https://go-review.googlesource.com/35251
Run-TryBot: Austin Clements <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Rick Hudson <[email protected]>
Reviewed-by: David Crawshaw <[email protected]>
mallocinit has evolved organically. Make a pass to clean it up in various ways: 1. Merge the computation of spansSize and bitmapSize. These were computed on every loop iteration of two different loops, but always have the same value, which can be derived directly from _MaxMem. This also avoids over-reserving these on MIPS, were _MaxArena32 is larger than _MaxMem. 2. Remove the ulimit -v logic. It's been disabled for many releases and the dead code paths to support it are even more wrong now than they were when it was first disabled, since now we *must* reserve spans and bitmaps for the full address space. 3. Make it clear that we're using a simple linear allocation to lay out the spans, bitmap, and arena spaces. Previously there were a lot of redundant pointer computations. Now we just bump p1 up as we reserve the spaces. In preparation for #18651. Updates #5049 (respect ulimit). Change-Id: Icbe66570d3a7a17bea227dc54fb3c4978b52a3af Reviewed-on: https://go-review.googlesource.com/35252 Reviewed-by: Russ Cox <[email protected]>
|
Change https://golang.org/cl/85887 mentions this issue: |
This replaces the contiguous heap arena mapping with a potentially sparse mapping that can support heap mappings anywhere in the address space. This has several advantages over the current approach: * There is no longer any limit on the size of the Go heap. (Currently it's limited to 512GB.) Hence, this fixes #10460. * It eliminates many failures modes of heap initialization and growing. In particular it eliminates any possibility of panicking with an address space conflict. This can happen for many reasons and even causes a low but steady rate of TSAN test failures because of conflicts with the TSAN runtime. See #16936 and #11993. * It eliminates the notion of "non-reserved" heap, which was added because creating huge address space reservations (particularly on 64-bit) led to huge process VSIZE. This was at best confusing and at worst conflicted badly with ulimit -v. However, the non-reserved heap logic is complicated, can race with other mappings in non-pure Go binaries (e.g., #18976), and requires that the entire heap be either reserved or non-reserved. We currently maintain the latter property, but it's quite difficult to convince yourself of that, and hence difficult to keep correct. This logic is still present, but will be removed in the next CL. * It fixes problems on 32-bit where skipping over parts of the address space leads to mapping huge (and never-to-be-used) metadata structures. See #19831. This also completely rewrites and significantly simplifies mheap.sysAlloc, which has been a source of many bugs. E.g., #21044, #20259, #18651, and #13143 (and maybe #23222). This change also makes it possible to allocate individual objects larger than 512GB. As a result, a few tests that expected huge allocations to fail needed to be changed to make even larger allocations. However, at the moment attempting to allocate a humongous object may cause the program to freeze for several minutes on Linux as we fall back to probing every page with addrspace_free. That logic (and this failure mode) will be removed in the next CL. Fixes #10460. Fixes #22204 (since it rewrites the code involved). This slightly slows down compilebench and the x/benchmarks garbage benchmark. name old time/op new time/op delta Template 184ms ± 1% 185ms ± 1% ~ (p=0.065 n=10+9) Unicode 86.9ms ± 3% 86.3ms ± 1% ~ (p=0.631 n=10+10) GoTypes 599ms ± 0% 602ms ± 0% +0.56% (p=0.000 n=10+9) Compiler 2.87s ± 1% 2.89s ± 1% +0.51% (p=0.002 n=9+10) SSA 7.29s ± 1% 7.25s ± 1% ~ (p=0.182 n=10+9) Flate 118ms ± 2% 118ms ± 1% ~ (p=0.113 n=9+9) GoParser 147ms ± 1% 148ms ± 1% +1.07% (p=0.003 n=9+10) Reflect 401ms ± 1% 404ms ± 1% +0.71% (p=0.003 n=10+9) Tar 175ms ± 1% 175ms ± 1% ~ (p=0.604 n=9+10) XML 209ms ± 1% 210ms ± 1% ~ (p=0.052 n=10+10) (https://perf.golang.org/search?q=upload:20171231.4) name old time/op new time/op delta Garbage/benchmem-MB=64-12 2.23ms ± 1% 2.25ms ± 1% +0.84% (p=0.000 n=19+19) (https://perf.golang.org/search?q=upload:20171231.3) Relative to the start of the sparse heap changes (starting at and including "runtime: fix various contiguous bitmap assumptions"), overall slowdown is roughly 1% on GC-intensive benchmarks: name old time/op new time/op delta Template 183ms ± 1% 185ms ± 1% +1.32% (p=0.000 n=9+9) Unicode 84.9ms ± 2% 86.3ms ± 1% +1.65% (p=0.000 n=9+10) GoTypes 595ms ± 1% 602ms ± 0% +1.19% (p=0.000 n=9+9) Compiler 2.86s ± 0% 2.89s ± 1% +0.91% (p=0.000 n=9+10) SSA 7.19s ± 0% 7.25s ± 1% +0.75% (p=0.000 n=8+9) Flate 117ms ± 1% 118ms ± 1% +1.10% (p=0.000 n=10+9) GoParser 146ms ± 2% 148ms ± 1% +1.48% (p=0.002 n=10+10) Reflect 398ms ± 1% 404ms ± 1% +1.51% (p=0.000 n=10+9) Tar 173ms ± 1% 175ms ± 1% +1.17% (p=0.000 n=10+10) XML 208ms ± 1% 210ms ± 1% +0.62% (p=0.011 n=10+10) [Geo mean] 369ms 373ms +1.17% (https://perf.golang.org/search?q=upload:20180101.2) name old time/op new time/op delta Garbage/benchmem-MB=64-12 2.22ms ± 1% 2.25ms ± 1% +1.51% (p=0.000 n=20+19) (https://perf.golang.org/search?q=upload:20180101.3) Change-Id: I5daf4cfec24b252e5a57001f0a6c03f22479d0f0 Reviewed-on: https://go-review.googlesource.com/85887 Run-TryBot: Austin Clements <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Rick Hudson <[email protected]>
Testing Go on an iPhone at 9cf06ed.
In a subsequent run I saw:
Seems to be a moderately reliable failure.
(it is failing much faster than the test harness suggests, in under a second.)
cc @aclements
The text was updated successfully, but these errors were encountered: