Skip to content

[release-1.11] malloc: use jl_get_current_task to fix null check #58202

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

Conversation

benlorenz
Copy link
Contributor

Since 1.11.5 we are getting crashes from OpenMP threads of an external library which uses GMP numbers. This did not happen on 1.11.4. Version 1.12 and nightly are also unaffected.

Attaching gdb shows that the task struct is an invalid pointer (but not NULL):

(gdb) p ct
$1 = (jl_task_t *) 0xffffffffffffff90

This value seems to come from the jl_current_task macro

#define jl_current_task (container_of(jl_get_pgcstack(), jl_task_t, gcstack))

which doesn't check whether jl_get_pgcstack() returns NULL for foreign threads. Two lines further down is a check for ct != NULL but this cannot happen since container_of will just subtract a fixed offset from a NULL-pointer.

julia/src/gc.c

Lines 4122 to 4124 in 2d89891

jl_task_t *ct = jl_current_task;
void *data = malloc(sz);
if (data != NULL && ct != NULL && ct->world_age) {

I changed the relevant two lines to use jl_get_current_task() which will properly return NULL if jl_get_pgcstack() returns NULL. This also aligns it with the current master:

julia/src/gc-stock.c

Lines 3741 to 3742 in 16eca6e

jl_task_t *ct = jl_get_current_task();
if (data != NULL && ct != NULL) {

This was probably introduced during the backport in #57880, @gbaraldi.

Note that this PR targets release-1.11, not sure if this is correct but there is no backports-release-1.11 branch right now.

cc: @lgoettgens
x-ref: oscar-system/Oscar.jl#4806

@lgoettgens
Copy link
Contributor

Note that this PR targets release-1.11, not sure if this is correct but there is no backports-release-1.11 branch right now.

Maybe @KristofferC can comment on this

@gbaraldi
Copy link
Member

LGTM branch target aside

@fingolfin fingolfin added the backport 1.11 Change should be backported to release-1.11 label Apr 24, 2025
@fingolfin
Copy link
Member

There doesn't seem to be a backports 1.11.6 branch at this point. I realize 1.12 is coming closer, but it would be really unfortunate to leave 1.11.5 as last 1.11.x release with such a severe regression.

@KristofferC
Copy link
Member

I'll make one soon

@KristofferC KristofferC mentioned this pull request Apr 25, 2025
42 tasks
@KristofferC KristofferC changed the base branch from release-1.11 to backports-release-1.11 April 25, 2025 09:36
@KristofferC KristofferC added the merge me PR is reviewed. Merge when all tests are passing label Apr 25, 2025
@IanButterworth IanButterworth merged commit 3997241 into JuliaLang:backports-release-1.11 Apr 28, 2025
9 checks passed
@benlorenz benlorenz deleted the bl/rel111_mallocnull branch April 28, 2025 22:00
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.11 Change should be backported to release-1.11
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants