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

Possible use after free in iddict rehashing function #52558

Closed
Zentrik opened this issue Dec 16, 2023 · 2 comments · Fixed by #52569
Closed

Possible use after free in iddict rehashing function #52558

Zentrik opened this issue Dec 16, 2023 · 2 comments · Fixed by #52569
Labels
bug Indicates an unexpected problem or unintended behavior

Comments

@Zentrik
Copy link
Member

Zentrik commented Dec 16, 2023

Is there a bug in the code that rehashes IdDict? Asan on #52496 is saying that ol[i+1] on line 23 is a use after being freed in the call to jl_alloc_memory_any (line 18).

julia/src/iddict.c

Lines 13 to 31 in 67c7843

JL_DLLEXPORT jl_genericmemory_t *jl_idtable_rehash(jl_genericmemory_t *a, size_t newsz)
{
size_t sz = a->length;
size_t i;
jl_value_t **ol = (jl_value_t **) a->ptr;
jl_genericmemory_t *newa = jl_alloc_memory_any(newsz);
// keep the original memory in the original slot since we need `ol`
// to be valid in the loop below.
JL_GC_PUSH2(&newa, &a);
for (i = 0; i < sz; i += 2) {
if (ol[i + 1] != NULL) {
jl_table_assign_bp(&newa, ol[i], ol[i + 1]);
// it is however necessary here because allocation
// can (and will) occur in a recursive call inside table_lookup_bp
}
}
JL_GC_POP();
return newa;
}

Should lines 18 and 21 be changed to the following,

    jl_genericmemory_t *newa = NULL;
    JL_GC_PUSH2(&newa, &a);
    newa = jl_alloc_memory_any(newsz);
@vtjnash
Copy link
Member

vtjnash commented Dec 17, 2023

Yes, that looks more correct

@oscardssmith oscardssmith added the bug Indicates an unexpected problem or unintended behavior label Dec 17, 2023
@oscardssmith
Copy link
Member

was this my fault? (from the Memory pr)

vchuravy pushed a commit that referenced this issue Dec 18, 2023
Should fix #52558. `a` should be rooted before the alloc call. I removed
the comment as it seemed to refer to a write barrier that was removed
long ago.
KristofferC pushed a commit that referenced this issue Jan 24, 2024
Should fix #52558. `a` should be rooted before the alloc call. I removed
the comment as it seemed to refer to a write barrier that was removed
long ago.

(cherry picked from commit 5977cb0)
Drvi pushed a commit to RelationalAI/julia that referenced this issue Jun 7, 2024
Should fix JuliaLang#52558. `a` should be rooted before the alloc call. I removed
the comment as it seemed to refer to a write barrier that was removed
long ago.

(cherry picked from commit 5977cb0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants