Skip to content

Commit

Permalink
Merge pull request #1381 from bettio/fix-write-register-gc-safe
Browse files Browse the repository at this point in the history 
Fix WRITE_REGISTER_GC_SAFE memory corruption

Fix #1379

In few words, `((dreg_gc_safe).base == x_regs)` is not true for
x registers >= 16.

Also there is a very minor optimization.

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
  • Loading branch information
@bettio
bettio committed Dec 11, 2024
2 parents 5967ad9 + 519194e commit 89438f3
Show file tree
Hide file tree
Showing 5 changed files with 95 additions and 11 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ might lead to a crash in certain situations.
- Fixed a memory leak where modules were not properly destroyed when the global context is destroyd
- alisp: fix support to variables that are not binaries or integers.
- Fixed destruction of ssl-related resources
- Fix corruption when dealing with specific situations that involve more than 16 x registers when
certain VM instructions are used.

## [0.6.5] - 2024-10-15

Expand Down
42 changes: 31 additions & 11 deletions src/libAtomVM/opcodesswitch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,23 @@ typedef struct
int index;
} dreg_gc_safe_t;

#define X_REG_FLAG 1

static inline term *with_x_reg_flag(term *xptr)
{
return (term *) (((uintptr_t) xptr) | X_REG_FLAG);
}

static inline bool has_x_reg_flag(term *xptr)
{
return ((uintptr_t) xptr) & X_REG_FLAG;
}

static inline term *to_x_reg_ptr(term *xptr)
{
return (term *) (((uintptr_t) xptr) & ~((uintptr_t) X_REG_FLAG));
}

static dreg_t extended_register_ptr(Context *ctx, unsigned int index)
{
struct ListHead *item;
Expand Down Expand Up @@ -549,7 +566,7 @@ static void destroy_extended_registers(Context *ctx, unsigned int live)

// TODO: fix register type heuristics, there is a chance that 'y' might be an "extended" x reg
#define T_DEST_REG_GC_SAFE(dreg_gc_safe) \
((dreg).base == x_regs) ? 'x' : 'y', ((dreg).index)
(has_x_reg_flag((dreg).base) ? 'x' : 'y'), ((dreg).index)

#define DECODE_COMPACT_TERM(dest_term, decode_pc) \
{ \
Expand Down Expand Up @@ -737,8 +754,9 @@ static void destroy_extended_registers(Context *ctx, unsigned int live)
#define READ_DEST_REGISTER(dreg) *(dreg)

#define READ_DEST_REGISTER_GC_SAFE(dreg_gc_safe) \
((dreg_gc_safe).base == x_regs ? x_regs[(dreg_gc_safe).index] : ctx->e[(dreg_gc_safe).index])

(has_x_reg_flag((dreg_gc_safe).base) ? \
*to_x_reg_ptr((dreg_gc_safe).base) : \
ctx->e[(dreg_gc_safe).index])

#define WRITE_REGISTER(dreg, value) \
{ \
Expand All @@ -747,8 +765,8 @@ static void destroy_extended_registers(Context *ctx, unsigned int live)

#define WRITE_REGISTER_GC_SAFE(dreg_gc_safe, value) \
{ \
if ((dreg_gc_safe).base == x_regs) { \
x_regs[(dreg_gc_safe).index] = value; \
if (has_x_reg_flag((dreg_gc_safe).base)) { \
*(to_x_reg_ptr((dreg_gc_safe).base)) = value; \
} else { \
ctx->e[(dreg_gc_safe).index] = value; \
} \
Expand Down Expand Up @@ -800,14 +818,16 @@ static void destroy_extended_registers(Context *ctx, unsigned int live)
} \
}

// NOTE: (dreg_gc_safe).index is initialized for x registers even if not used to avoid compiler
// warnings about unitialized variables (-maybe-uninitialized)
#define DECODE_DEST_REGISTER_GC_SAFE(dreg_gc_safe, decode_pc) \
{ \
uint8_t first_byte = *(decode_pc)++; \
uint8_t reg_type = first_byte & 0xF; \
uint8_t reg_index = (first_byte >> 4); \
switch (reg_type) { \
case COMPACT_XREG: \
(dreg_gc_safe).base = x_regs; \
(dreg_gc_safe).base = with_x_reg_flag(&x_regs[reg_index]); \
(dreg_gc_safe).index = reg_index; \
break; \
case COMPACT_YREG: \
Expand All @@ -821,8 +841,8 @@ static void destroy_extended_registers(Context *ctx, unsigned int live)
if (IS_NULL_PTR(reg_base)) { \
RAISE_ERROR(OUT_OF_MEMORY_ATOM); \
} \
(dreg_gc_safe).base = reg_base; \
(dreg_gc_safe).index = 0; \
(dreg_gc_safe).base = with_x_reg_flag(reg_base); \
(dreg_gc_safe).index = reg_index; \
} else { \
VM_ABORT(); \
} \
Expand Down Expand Up @@ -6210,8 +6230,8 @@ HOT_FUNC int scheduler_entry_point(GlobalContext *glb)

#if MAXIMUM_OTP_COMPILER_VERSION >= 22
case OP_PUT_TUPLE2: {
GC_SAFE_DEST_REGISTER(dreg);
DECODE_DEST_REGISTER_GC_SAFE(dreg, pc);
DEST_REGISTER(dreg);
DECODE_DEST_REGISTER(dreg, pc);
DECODE_EXTENDED_LIST_TAG(pc);
uint32_t size;
DECODE_LITERAL(size, pc)
Expand All @@ -6237,7 +6257,7 @@ HOT_FUNC int scheduler_entry_point(GlobalContext *glb)
}

#ifdef IMPL_EXECUTE_LOOP
WRITE_REGISTER_GC_SAFE(dreg, t);
WRITE_REGISTER(dreg, t);
#endif
break;
}
Expand Down
2 changes: 2 additions & 0 deletions tests/erlang_tests/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,7 @@ compile_erlang(test_utf8_atoms)
compile_erlang(twentyone_param_function)
compile_erlang(complex_list_match_xregs)
compile_erlang(twentyone_param_fun)
compile_erlang(gc_safe_x_reg_write)

compile_erlang(test_fun_to_list)
compile_erlang(maps_nifs)
Expand Down Expand Up @@ -981,6 +982,7 @@ add_custom_target(erlang_test_modules DEPENDS
twentyone_param_function.beam
complex_list_match_xregs.beam
twentyone_param_fun.beam
gc_safe_x_reg_write.beam

test_fun_to_list.beam
maps_nifs.beam
Expand Down
59 changes: 59 additions & 0 deletions tests/erlang_tests/gc_safe_x_reg_write.erl
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
%
% This file is part of AtomVM.
%
% Copyright 2024 Jakub Gonet <[email protected]>
% Copyright 2024 Davide Bettio <[email protected]>
%
% Licensed under the Apache License, Version 2.0 (the "License");
% you may not use this file except in compliance with the License.
% You may obtain a copy of the License at
%
% http://www.apache.org/licenses/LICENSE-2.0
%
% Unless required by applicable law or agreed to in writing, software
% distributed under the License is distributed on an "AS IS" BASIS,
% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
% See the License for the specific language governing permissions and
% limitations under the License.
%
% SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later
%

% This test has been made after https://github.com/atomvm/AtomVM/issues/1379

-module(gc_safe_x_reg_write).
-export([start/0]).
-export([f/0, check/2]).

start() ->
?MODULE:check(?MODULE:f(), 0) - 21.

f() ->
[
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0},
{f, fun f/0}
].

check([], Count) ->
Count;
check([{f, F} | T], Count) when is_function(F) ->
check(T, Count + 1).
1 change: 1 addition & 0 deletions tests/test.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,7 @@ struct Test tests[] = {
TEST_CASE(twentyone_param_function),
TEST_CASE(complex_list_match_xregs),
TEST_CASE(twentyone_param_fun),
TEST_CASE(gc_safe_x_reg_write),

TEST_CASE(test_fun_to_list),
TEST_CASE(maps_nifs),
Expand Down

0 comments on commit 89438f3

Please sign in to comment.