Fix nif_atomvm_posix_read GC bug #1601
Conversation
src/libAtomVM/posix_nifs.c
Outdated
| int count = term_to_int(count_term); | ||
|
|
||
| size_t size = term_binary_data_size_in_terms(count) + BINARY_HEADER_SIZE + TERM_BOXED_SUB_BINARY_SIZE + TUPLE_SIZE(2); | ||
| if (UNLIKELY(memory_ensure_free_with_roots(ctx, size, 2, argv, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) { |
There was a problem hiding this comment.
We do not need to keep argv[1] in roots AFAIK
There was a problem hiding this comment.
Yup, but I thought it would be more future-proof to put all the args in the roots, and it shouldn't have noticeable performance overhead. WDYT?
There was a problem hiding this comment.
in that specific case, if you just want to future proof it would be better to have argc, argv otherwise let's use 1.
bettio
left a comment
bettio
left a comment
There was a problem hiding this comment.
This bug is likely also in release-0.6: let's rebase this PR and metion this on our changelog under v0.6.6 / Fixed sections.
mat-hek
force-pushed
the
upstream-fix-posix-read
branch
2 times, most recently
from
3bb7b06 to
804de39
Compare
mat-hek
force-pushed
the
upstream-fix-posix-read
branch
2 times, most recently
from
40c12af to
1d00d27
Compare
bettio
left a comment
bettio
left a comment
There was a problem hiding this comment.
Everything good. However I didn't notice that you didn't --signoff your code (see also git commit --signoff) and CI failed.
Signed-off-by: Mateusz Front <[email protected]>
mat-hek
force-pushed
the
upstream-fix-posix-read
branch
from
1d00d27 to
0c8c1a5
Compare
3 participants
We've been getting ASAN errors in the calls to
nif_atomvm_posix_read, and it seems it's because when GC moves things around, it also moves what thefd_obj_ptrvariable points to. The proposed solution is to move the GC beforeenif_get_resource. We haven't encountered the error after the fix, but please verify that it makes sense.These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later