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.

Sign up for GitHub

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

Fix memory corruption if task is launched inside finalizer #50597

Merged
merged 1 commit into from
Jul 19, 2023
Merged

Fix memory corruption if task is launched inside finalizer #50597

merged 1 commit into from
Jul 19, 2023

Conversation

Keno
Copy link
Member

@Keno Keno commented Jul 19, 2023

In #48919, the tid selection logic inside enq_task gained a !GC.in_finalizer() condition. However, this made it possible for workqueue_at to be reached with tid==0, which would attempt and out-of-bounds write under @inbounds, corrupting memory. This was not caught in the test suite despite --check-bounds=yes, because our --check-bounds=yes is currently best effort. That would be fixed by #50239, which exposed this bug. This PR attempts to
fix this by marking any tasks launched inside a finalizer as not sticky. Finalizers don't have any thread they run on
semantically, so i don't think there's a meaningful sense in which tasks launched inside finalizers could be sticky.

@Keno
Fix memory corruption if task is launched inside finalizer
2899656 
In #48919, the tid selection logic inside `enq_task` gained a
`!GC.in_finalizer()` condition. However, this made it possible
for `workqueue_at` to be reached with `tid==0`, which would
attempt and out-of-bounds write under `@inbounds`, corrupting
memory. This was not caught in the test suite despite
`--check-bounds=yes`, because our `--check-bounds=yes` is currently
best effort. That would be fixed by #50239, which exposed this
bug.
@Keno Keno requested a review from jpsamaroo July 19, 2023 13:58
@oscardssmith
Copy link
Member

Is it legal to spawn tasks in a finalizer?

@Keno
Copy link
Member Author

Keno commented Jul 19, 2023

Yes

@quinnj
Copy link
Member

quinnj commented Jul 19, 2023

Is it legal to spawn tasks in a finalizer?

It's even recommended in the finalizer docs

@gbaraldi
Copy link
Member

Can we test this?

@Keno
Copy link
Member Author

Keno commented Jul 19, 2023

We have tests for this, they're just not working until we do #50239 or equivalent.

vtjnash
vtjnash approved these changes Jul 19, 2023
View reviewed changes
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add @assert tid > 0 to workqueue_for slow path?

@Keno Keno merged commit bd8350b into master Jul 19, 2023
1 check passed
@Keno Keno deleted the kf/dontcorruptmemory branch July 19, 2023 19:37
Keno added a commit that referenced this pull request Jul 19, 2023
@Keno
Assert tid>0 in workqueue_for
ae153d1 
Otherwise the inbounds annotations are not sound. As requested in #50597 (review).
@Keno Keno mentioned this pull request Jul 19, 2023
Merged
@maleadt
Copy link
Member

maleadt commented Jul 19, 2023

Backport to 1.10?

@oscardssmith oscardssmith added the backport 1.10 Change should be backported to the 1.10 release label Jul 19, 2023
Keno added a commit that referenced this pull request Jul 22, 2023
@Keno
Assert tid>0 in workqueue_for (#50602)
d1be33d 
Otherwise the inbounds annotations are not sound. As requested in
#50597 (review).
KristofferC pushed a commit that referenced this pull request Jul 24, 2023
@Keno @KristofferC
Fix memory corruption if task is launched inside finalizer (#50597)
a2fda25 
In #48919, the tid selection logic inside `enq_task` gained a
`!GC.in_finalizer()` condition. However, this made it possible for
`workqueue_at` to be reached with `tid==0`, which would attempt and
out-of-bounds write under `@inbounds`, corrupting memory. This was not
caught in the test suite despite `--check-bounds=yes`, because our
`--check-bounds=yes` is currently best effort. That would be fixed by
#50239, which exposed this bug. This PR attempts to
fix this by marking any tasks launched inside a finalizer as not sticky.
Finalizers don't have any thread they run on
semantically, so i don't think there's a meaningful sense in which tasks
launched inside finalizers could be sticky.

(cherry picked from commit bd8350b)
@KristofferC KristofferC mentioned this pull request Jul 24, 2023
Merged
45 tasks
KristofferC added a commit that referenced this pull request Jul 24, 2023
@KristofferC
release-1.10: Backports for 1.10-alpha2 (#50508)
f9b2614 
Backported PRs:
- [x] #50411 <!-- Fix weird dispatch of * with zero arguments -->
- [x] #50202 <!-- Remove dynamic dispatch from _wait/wait2 -->
- [x] #50064 <!-- Fix numbered prompt with input only with comment -->
- [x] #50026 <!-- Store heapsnapshot files in tempdir() instead of
current directory -->
- [x] #50402 <!-- Add CPU feature helper function -->
- [x] #50387 <!-- update newpages pointer after actually sweeping pages
-->
- [x] #50424 <!-- avoid potential type-instability in _replace_(str,
...) -->
- [x] #50444 <!-- Optimize getfield lowering to avoid boxing in some
cases -->
- [x] #50474 <!-- docs: Fix a `!!! note` which was miscapitalized -->
- [x] #50466 <!-- relax assertion involving pg->nold to reflect that it
may be a bit in… -->
- [x] #50490 <!-- Fix compat annotation for italic printstyled -->
- [x] #50488 <!-- fix typo in `Base.isassigned` with `Tridiagonal` -->
- [x] #50476 <!-- Profile: Add specifying dir for `take_heap_snapshot`
and handling if current dir is unwritable -->
- [x] #50461 <!-- fix typo in the --gcthreads argument description -->
- [x] #50528 <!-- ssair: Correctly handle stmt insertion at end of basic
block -->
- [x] #50533 <!-- ensure internal_obj_base_ptr checks whether objects
past freelist pointer are in freelist -->
- [x] #49322 <!-- improve cat design / performance -->
- [x] #50540 <!-- gc: remove over-eager assertion -->
- [x] #50542 <!-- gf: remove unnecessary assert cycle==depth -->
- [x] #50559 <!-- Expand kwcall lowering positional default check to
vararg -->
- [x] #50058 <!-- Add unwrapping mechanism for triangular mul and solves
-->
- [x] #50551 <!-- typeintersect: also record chained `innervars` -->
- [x] #50552 <!-- read(io, Char): fix read with too many leading ones
-->
- [x] #50541 <!-- precompile: ensure globals are not accidentally
created where disallowed -->
- [x] #50576 <!-- use atomic compare exchange when setting the GC
mark-bit -->
- [x] #50578 <!-- gf: make method overwrite/delete an error during
precompile -->
- [x] #50516 <!-- Fix visibility of assert on GCC12/13 -->
- [x] #50597 <!-- Fix memory corruption if task is launched inside
finalizer -->
- [x] #50591 <!-- build: fix various makefile bugs -->
- [x] #50599 <!-- faster invalid object lookup in conservative gc -->
- [x] #50634 <!-- 🤖 [master] Bump the SparseArrays stdlib from b4b0e72
to 99c99b4 -->
- [x] #50639 <!-- Backport LLVM patches to fix various issues. -->
- [x] #50546 <!-- Revert storage of method instance in LineInfoNode -->
- [x] #50631 <!-- Shift DCE pass to optimize imaging mode code better
-->
- [x] #50525 <!-- only check that values are finite in `generic_lufact`
when `check=true` -->
- [x] #50587 <!-- isassigned for ranges with BigInt indices -->
- [x] #50144 <!-- Page based heap size heuristics -->


Need manual backport:
- [ ] #50595 <!-- Rename ENV variable `JULIA_USE_NEW_PARSER` ->
`JULIA_USE_FLISP_PARSER` -->



Non-merged PRs with backport label:
- [ ] #50637 <!-- Remove SparseArrays legacy code -->
- [ ] #50618 <!-- inference: continue const-prop' when concrete-eval
returns non-inlineable -->
- [ ] #50598 <!-- only limit types in stack traces in the REPL -->
- [ ] #50594 <!-- Disallow non-index Integer types in isassigned -->
- [ ] #50568 <!-- `Array(::AbstractRange)` should return an `Array` -->
- [ ] #50523 <!-- Avoid generic call in most cases for getproperty -->
- [ ] #50172 <!-- print feature flags used for matching pkgimage -->
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vchuravy vchuravy vchuravy approved these changes

@vtjnash vtjnash vtjnash approved these changes

@jpsamaroo jpsamaroo Awaiting requested review from jpsamaroo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Keno @oscardssmith @quinnj @gbaraldi @maleadt @vchuravy @vtjnash @KristofferC