Skip to content

Commit

Permalink
Fix crash in socket refc handling
Browse files Browse the repository at this point in the history 
Ensure refcount of socket resources is properly handled on demonitor
Also ensure that demonitor happens when selected process is cleared

Signed-off-by: Paul Guyot <pguyot@kallisys.net>
  • Loading branch information
@pguyot
pguyot committed Jan 17, 2025
1 parent f802020 commit dd552b0
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 27 deletions.
2 changes: 1 addition & 1 deletion src/libAtomVM/globalcontext.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ COLD_FUNC void globalcontext_destroy(GlobalContext *glb)
struct RefcBinary *refc = GET_LIST_ENTRY(item, struct RefcBinary, head);
#ifndef NDEBUG
if (refc->resource_type) {
fprintf(stderr, "Warning, dangling resource of type %s, ref_count = %d\n", refc->resource_type->name, (int) refc->ref_count);
fprintf(stderr, "Warning, dangling resource of type %s, ref_count = %d, data = %p\n", refc->resource_type->name, (int) refc->ref_count, refc->data);
} else {
fprintf(stderr, "Warning, dangling refc binary, ref_count = %d\n", (int) refc->ref_count);
}
Expand Down
68 changes: 42 additions & 26 deletions src/libAtomVM/otp_socket.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include <otp_socket.h>
#include <port.h>
#include <posix_nifs.h>
#include <refc_binary.h>
#include <scheduler.h>
#include <smp.h>
#include <sys.h>
Expand Down Expand Up @@ -285,6 +286,8 @@ static void socket_stop(ErlNifEnv *caller_env, void *obj, ErlNifEvent event, int
if (rsrc_obj->selecting_process_id != INVALID_PROCESS_ID) {
enif_demonitor_process(caller_env, rsrc_obj, &rsrc_obj->selecting_process_monitor);
rsrc_obj->selecting_process_id = INVALID_PROCESS_ID;
struct RefcBinary *rsrc_refc = refc_binary_from_data(rsrc_obj);
refc_binary_decrement_refcount(rsrc_refc, caller_env->global);
}

TRACE("socket_stop called on fd=%i\n", rsrc_obj->fd);
Expand All @@ -305,42 +308,43 @@ static void socket_down(ErlNifEnv *caller_env, void *obj, ErlNifPid *pid, ErlNif
TRACE("socket_down called on process_id=%i\n", (int) *pid);
#endif

// Increment the reference count so the resource doesn't go away
// (enif_select will decrement the ref count)
struct RefcBinary *rsrc_refc = refc_binary_from_data(rsrc_obj);
refc_binary_increment_refcount(rsrc_refc);
SMP_RWLOCK_WRLOCK(rsrc_obj->socket_lock);

#if OTP_SOCKET_BSD
if (rsrc_obj->selecting_process_id != INVALID_PROCESS_ID) {
// Monitor fired, so make sure we don't try to demonitor in select_stop
// as it could crash trying to reacquire lock on process table
rsrc_obj->selecting_process_id = INVALID_PROCESS_ID;
enif_select(caller_env, rsrc_obj->fd, ERL_NIF_SELECT_STOP, rsrc_obj, NULL, term_nil());
if (rsrc_obj->selecting_process_id == INVALID_PROCESS_ID) {
SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock);
return;
}

#if OTP_SOCKET_BSD
// Monitor fired, so make sure we don't try to demonitor in select_stop
// as it could crash trying to reacquire lock on process table
// enif_select can decrement ref count but it's at least 2 in this case (1 for monitor and 1 for select)
rsrc_obj->selecting_process_id = INVALID_PROCESS_ID;
enif_select(caller_env, rsrc_obj->fd, ERL_NIF_SELECT_STOP, rsrc_obj, NULL, term_nil());
#elif OTP_SOCKET_LWIP
// Monitor can be called when we're selecting, accepting or connecting.
if (rsrc_obj->selecting_process_id != INVALID_PROCESS_ID) {
LWIP_BEGIN();
rsrc_obj->selecting_process_id = INVALID_PROCESS_ID;
if (rsrc_obj->socket_state & SocketStateTCP) {
if (rsrc_obj->socket_state & SocketStateTCPListening) {
(void) tcp_close(rsrc_obj->tcp_pcb);
} else {
tcp_abort(rsrc_obj->tcp_pcb);
}
rsrc_obj->tcp_pcb = NULL;
rsrc_obj->socket_state = SocketStateClosed;
} else if (rsrc_obj->socket_state & SocketStateUDP) {
udp_remove(rsrc_obj->udp_pcb);
rsrc_obj->udp_pcb = NULL;
rsrc_obj->socket_state = SocketStateClosed;
LWIP_BEGIN();
rsrc_obj->selecting_process_id = INVALID_PROCESS_ID;
if (rsrc_obj->socket_state & SocketStateTCP) {
if (rsrc_obj->socket_state & SocketStateTCPListening) {
(void) tcp_close(rsrc_obj->tcp_pcb);
} else {
tcp_abort(rsrc_obj->tcp_pcb);
}
LWIP_END();
rsrc_obj->tcp_pcb = NULL;
rsrc_obj->socket_state = SocketStateClosed;
} else if (rsrc_obj->socket_state & SocketStateUDP) {
udp_remove(rsrc_obj->udp_pcb);
rsrc_obj->udp_pcb = NULL;
rsrc_obj->socket_state = SocketStateClosed;
}
LWIP_END();
#endif

SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock);

// We're no longer monitoring so we can decrement ref count
refc_binary_decrement_refcount(rsrc_refc, caller_env->global);
}

Expand Down Expand Up @@ -948,13 +952,16 @@ static term nif_socket_select_read(Context *ctx, int argc, term argv[])
RAISE_ERROR(BADARG_ATOM);
}

struct RefcBinary *rsrc_refc = refc_binary_from_data(rsrc_obj);
SMP_RWLOCK_WRLOCK(rsrc_obj->socket_lock);

ErlNifEnv *env = erl_nif_env_from_context(ctx);
if (rsrc_obj->selecting_process_id != ctx->process_id && rsrc_obj->selecting_process_id != INVALID_PROCESS_ID) {
// demonitor can fail if process is gone.
enif_demonitor_process(env, rsrc_obj, &rsrc_obj->selecting_process_monitor);
rsrc_obj->selecting_process_id = INVALID_PROCESS_ID;
// decrement ref count as we are demonitoring
refc_binary_decrement_refcount(rsrc_refc, ctx->global);
}
// Monitor first as select is less likely to fail and it's less expensive to demonitor
// if select fails than to stop select if monitor fails
Expand All @@ -963,6 +970,8 @@ static term nif_socket_select_read(Context *ctx, int argc, term argv[])
SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock);
RAISE_ERROR(NOPROC_ATOM);
}
// increment ref count so the resource doesn't go away until monitor is fired
refc_binary_increment_refcount(rsrc_refc);
rsrc_obj->selecting_process_id = ctx->process_id;
}

Expand All @@ -979,6 +988,7 @@ static term nif_socket_select_read(Context *ctx, int argc, term argv[])
enif_demonitor_process(env, rsrc_obj, &rsrc_obj->selecting_process_monitor);
rsrc_obj->selecting_process_id = INVALID_PROCESS_ID;
SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock);
refc_binary_decrement_refcount(rsrc_refc, ctx->global);
RAISE_ERROR(BADARG_ATOM);
}
}
Expand Down Expand Up @@ -1025,6 +1035,7 @@ static term nif_socket_select_read(Context *ctx, int argc, term argv[])
enif_demonitor_process(env, rsrc_obj, &rsrc_obj->selecting_process_monitor);
LWIP_END();
SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock);
refc_binary_decrement_refcount(rsrc_refc, ctx->global);
RAISE_ERROR(BADARG_ATOM);
}
LWIP_END();
Expand All @@ -1047,7 +1058,12 @@ static term nif_socket_select_stop(Context *ctx, int argc, term argv[])
}
// Avoid the race condition with select object here.
SMP_RWLOCK_WRLOCK(rsrc_obj->socket_lock);
rsrc_obj->selecting_process_id = INVALID_PROCESS_ID;
if (rsrc_obj->selecting_process_id != INVALID_PROCESS_ID) {
enif_demonitor_process(erl_nif_env_from_context(ctx), rsrc_obj, &rsrc_obj->selecting_process_monitor);
rsrc_obj->selecting_process_id = INVALID_PROCESS_ID;
struct RefcBinary *rsrc_refc = refc_binary_from_data(rsrc_obj);
refc_binary_decrement_refcount(rsrc_refc, ctx->global);
}
#if OTP_SOCKET_BSD
if (UNLIKELY(enif_select(erl_nif_env_from_context(ctx), rsrc_obj->fd, ERL_NIF_SELECT_STOP, rsrc_obj, NULL, term_nil()) < 0)) {
RAISE_ERROR(BADARG_ATOM);
Expand Down

0 comments on commit dd552b0

Please sign in to comment.