Revert "Don't expose guard pages to malloc_stack API consumers" #55555
Conversation
This reverts commit 5dfd57d.
|
Cc @fingolfin |
|
I have a hard time understanding the PR description, probably because I am misunderstanding something fundamental, sorry :-/. So some questions:
What are these consumers? The description makes it sound as if everything should burn and explode, but at least the Julia test suite seemed to pass.
I guess what is "correct" depends on the definition of the interface. I read this as saying: "the semantics of
I have trouble following you at all here. Access which pages? Which redzones (is that ASAN terminology)? Anyway: if this is reverted, then I'll dust of my old alternative patch, which modifies Just to explain, the goal of the removed patch was to to simplify scanning task stacks (for a conservative GC) for references to objects. For that we want to stop before hitting the guard page. |
|
(To be clear, I am not objecting to the reversal, I just would like to understand to make sure there is not some deeper issue in my stack scanning code) |
|
The task scanning should stop once it hits the current stack pointer, and never go all of the way to reaching these addresses. The code elsewhere detects if faults occur on the stack and treats them differently than other segfaults. |
|
The redzone is the number of bytes past the current stack pointer that are also used by the function (https://en.wikipedia.org/wiki/Red_zone_(computing)) |
|
Yes, I agree --- you should only scan up to the actual stack pointer, and excluding guard pages was just a hack to attempt to make scanning work without knowing the stack pointer. |
|
Also, |
|
We'd be happy to only scan to the stack pointer, but AFAIK so far it isn't available, or is it now? We've performed the conservative scanning of task stacks for many years now, and it worked well enough. |
|
All the code in base (e.g. the unmap here and signal handling elsewhere) wants to know the full size of the memory region being managed, based upon the observed need to add |
| void* stk = mmap(0, bufsz, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); | ||
| if (stk == MAP_FAILED) | ||
| return MAP_FAILED; | ||
|
|
||
| #ifdef JL_USE_GUARD_PAGE |
There was a problem hiding this comment.
This used to be #if !defined(JL_HAVE_UCONTEXT) && !defined(JL_HAVE_SIGALTSTACK) before #54591
There was a problem hiding this comment.
Yes, those changed in a subsequent PR that I didn't want to revert
Reverts #54591 This cause the runtime to misbehave and crash, since all of the consumers of this information in the runtime assumed that the guard pages are accounted for correctly as part of the reserved allocation. Nothing in the runtime ever promised that it is valid to access the pages beyond the current redzone (indeed, ASAN would forbid it as well).
Backported PRs: - [x] #55480 <!-- Fix push! for OffsetVectors, add tests for push! and append! on AbstractVector --> - [x] #55443 <!-- Add test for upper/lower/titlecase and fix call --> - [x] #55524 <!-- Set `.jl` sources as read-only during installation --> - [x] #55500 <!-- make jl_thread_suspend_and_get_state safe --> - [x] #55506 <!-- Fix indexing in _mapreducedim for OffsetArrays --> - [x] #55564 <!-- Empty out loaded_precompiles dict instead of asserting it's empty. --> - [x] #55567 <!-- Initialize threadpools correctly during sysimg build --> - [x] #55596 <!-- Fast bounds-check for CartesianIndex ranges --> - [x] #55605 <!-- Reroute Symmetric/Hermitian + Diagonal through triangular --> - [x] #55640 <!-- win: move stack_overflow_warning to the backtrace fiber --> - [x] #55715 <!-- Add precompile signatures to Markdown to reduce latency. --> - [x] #55593 <!-- Fix invalidations for FileIO --> - [x] #55555 <!-- Revert "Don't expose guard pages to malloc_stack API consumers" --> - [x] #55720 <!-- Fix `pkgdir` for extensions --> - [x] #55729 <!-- Avoid confounding compilation side effects of `@time_imports` --> - [x] #55718 <!-- Fix `@time_imports` extension recognition --> - [x] #55522 <!-- Fix tr for Symmetric/Hermitian block matrices --> Contains multiple commits, manual intervention needed: - [ ] #55509 <!-- Fix cong implementation to be properly random and not just cycling. --> Non-merged PRs with backport label: - [ ] #55641 <!-- fall back to slower stat filesize if optimized filesize fails --> - [ ] #55534 <!-- Set stdlib sources as read-only during installation --> - [ ] #55499 <!-- propagate the terminal's `displaysize` to the `IOContext` used by the REPL --> - [ ] #55458 <!-- Allow for generically extracting unannotated string --> - [ ] #55457 <!-- Make AnnotateChar equality consider annotations --> - [ ] #55453 <!-- Privatise the annotations API, for StyledStrings --> - [ ] #55355 <!-- relocation: account for trailing path separator in depot paths --> - [ ] #55220 <!-- `isfile_casesensitive` fixes on Windows --> - [ ] #55169 <!-- `propertynames` for SVD respects private argument --> - [ ] #54457 <!-- Make `String(::Memory)` copy --> - [ ] #53957 <!-- tweak how filtering is done for what packages should be precompiled --> - [ ] #51479 <!-- prevent code loading from lookin in the versioned environment when building Julia --> - [ ] #50813 <!-- More doctests for Sockets and capitalization fix --> - [ ] #50157 <!-- improve docs for `@inbounds` and `Base.@propagate_inbounds` --> - [ ] #41244 <!-- Fix shell `cd` error when working dir has been deleted -->
KristofferC
removed
the
backport 1.11
Reverts #54591
This cause the runtime to misbehave and crash, since all of the consumers of this information in the runtime assumed that the guard pages are accounted for correctly as part of the reserved allocation. Nothing in the runtime ever promised that it is valid to access the pages beyond the current redzone (indeed, ASAN would forbid it as well).