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

Fix missing GC root in Symbol construction #47865

Merged
merged 1 commit into from
Dec 11, 2022
Merged

Fix missing GC root in Symbol construction #47865

merged 1 commit into from
Dec 11, 2022

Conversation

Keno
Copy link
Member

@Keno Keno commented Dec 11, 2022

The Symbol constructor in boot.jl was not using the unsafe_convert mechanism, becuase it is unavailable at this point in bootstrap. However, it was also not GC-rooting the string some other way, resulting in potential memory corruption. Fix that by manually inlining the :foreigncall and setting up the root appropriately.

The `Symbol` constructor in boot.jl was not using the unsafe_convert mechanism,
becuase it is unavailable at this point in bootstrap. However, it was also not
GC-rooting the string some other way, resulting in potential memory corruption.
Fix that by manually inlining the :foreigncall and setting up the root
appropriately.
@brenhinkeller brenhinkeller added the GC Garbage collector label Dec 11, 2022
@vchuravy vchuravy added backport 1.8 Change should be backported to release-1.8 backport 1.9 Change should be backported to release-1.9 labels Dec 11, 2022
@vtjnash
Copy link
Member

vtjnash commented Dec 11, 2022

SGTM. Though I wonder if we shouldn't just duplicate the code, since the random _Symbol function feels a bit awkward for something needed exactly twice ever

@Keno Keno merged commit b5a6b0f into master Dec 11, 2022
@Keno Keno deleted the kf/symbolgcroot branch December 11, 2022 20:34
@Keno
Copy link
Member Author

Keno commented Dec 11, 2022

Though I wonder if we shouldn't just duplicate the code, since the random _Symbol function feels a bit awkward for something needed exactly twice ever

I did it that way at first, but then I wanted to minimize the number of manually written :foreigncall exprs, since that representation is not stable. Of course, in the end it doesn't really matter either way. If you'd like it switched, I'm happy to do that.

@vtjnash
Copy link
Member

vtjnash commented Dec 12, 2022

That is also true. I guess I also like to minimize the number of APIs that take naked Ptr to ccall something from them, but this PR is fine

@KristofferC KristofferC mentioned this pull request Dec 14, 2022
26 tasks
@KristofferC
Copy link
Member

This segfaults when backporting to 1.8 when making corecompiler.jl:

    JULIA usr/lib/julia/corecompiler.ji
Segmentation fault (core dumped)
make[1]: *** [sysimage.mk:61: /home/kc/julia/usr/lib/julia/corecompiler.ji] Error 139
make: *** [Makefile:82: julia-sysimg-ji] Error 2

KristofferC pushed a commit that referenced this pull request Dec 14, 2022
The `Symbol` constructor in boot.jl was not using the unsafe_convert mechanism,
becuase it is unavailable at this point in bootstrap. However, it was also not
GC-rooting the string some other way, resulting in potential memory corruption.
Fix that by manually inlining the :foreigncall and setting up the root
appropriately.

(cherry picked from commit b5a6b0f)
@KristofferC KristofferC mentioned this pull request Dec 27, 2022
10 tasks
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Dec 27, 2022
@KristofferC KristofferC mentioned this pull request Feb 6, 2023
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.8 Change should be backported to release-1.8 GC Garbage collector
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants