From 7923c2c7ce4deba75d845501eb96ddce07dc5ea0 Mon Sep 17 00:00:00 2001 From: Micael Karlberg Date: Thu, 6 Dec 2018 12:40:37 +0100 Subject: [socket-nif] Valgrind: plugged memory leaks in nif_recv Running valgrind, found a couple of cases when allocated binaries where not released. Mostly when the recv failed for some reason. Also clear and free the environment associated with the socket (resource) (in the _dtor callback function). OTP-14831 --- erts/emulator/nifs/common/socket_int.h | 1 + erts/emulator/nifs/common/socket_nif.c | 41 ++++++++++++++++++++++++++++------ 2 files changed, 35 insertions(+), 7 deletions(-) (limited to 'erts/emulator/nifs/common') diff --git a/erts/emulator/nifs/common/socket_int.h b/erts/emulator/nifs/common/socket_int.h index 7d223b8259..ec17e45f25 100644 --- a/erts/emulator/nifs/common/socket_int.h +++ b/erts/emulator/nifs/common/socket_int.h @@ -378,6 +378,7 @@ extern ERL_NIF_TERM esock_atom_einval; #define ALLOC_BIN(SZ, BP) enif_alloc_binary((SZ), (BP)) #define REALLOC_BIN(SZ, BP) enif_realloc_binary((SZ), (BP)) +#define FREE_BIN(BP) enif_release_binary((BP)) #endif // SOCKET_INT_H__ diff --git a/erts/emulator/nifs/common/socket_nif.c b/erts/emulator/nifs/common/socket_nif.c index 3da895e644..57ce92d77e 100644 --- a/erts/emulator/nifs/common/socket_nif.c +++ b/erts/emulator/nifs/common/socket_nif.c @@ -4233,8 +4233,13 @@ ERL_NIF_TERM nopen(ErlNifEnv* env, descP->type = type; descP->protocol = protocol; + /* + * Should we keep track of sockets (resources) in some way? + * Doing it here will require mutex to ensure data integrity, + * which will be costly. Send it somewhere? + */ res = enif_make_resource(env, descP); - enif_release_resource(descP); // We should really store a reference ... + enif_release_resource(descP); /* Keep track of the creator * This should not be a problem but just in case @@ -13592,6 +13597,8 @@ ERL_NIF_TERM recv_check_result(ErlNifEnv* env, recv_error_current_reader(env, descP, res); + FREE_BIN(bufP); + return res; } @@ -13633,6 +13640,9 @@ ERL_NIF_TERM recv_check_result(ErlNifEnv* env, if ((xres = recv_init_current_reader(env, descP, recvRef)) != NULL) return esock_make_error_str(env, xres); + /* This transfers "ownership" of the *allocated* binary to an + * erlang term (no need for an explicit free). + */ data = MKBIN(env, bufP); SSDBG( descP, @@ -13664,6 +13674,9 @@ ERL_NIF_TERM recv_check_result(ErlNifEnv* env, recv_update_current_reader(env, descP); + /* This transfers "ownership" of the *allocated* binary to an + * erlang term (no need for an explicit free). + */ data = MKBIN(env, bufP); return esock_make_ok3(env, atom_true, data); @@ -13707,6 +13720,8 @@ ERL_NIF_TERM recv_check_result(ErlNifEnv* env, (ERL_NIF_SELECT_STOP), descP, NULL, recvRef); + FREE_BIN(bufP); + return res; } else if ((saveErrno == ERRNO_BLOCK) || @@ -13721,16 +13736,14 @@ ERL_NIF_TERM recv_check_result(ErlNifEnv* env, SSDBG( descP, ("SOCKET", "recv_check_result -> SELECT for more\r\n") ); - /* - SELECT(env, descP->sock, (ERL_NIF_SELECT_READ), - descP, NULL, recvRef); - */ - sres = enif_select(env, descP->sock, (ERL_NIF_SELECT_READ), descP, NULL, recvRef); - SSDBG( descP, ("SOCKET", "recv_check_result -> SELECT res: %d\r\n", sres) ); + SSDBG( descP, + ("SOCKET", "recv_check_result -> SELECT res: %d\r\n", sres) ); + FREE_BIN(bufP); + return esock_make_error(env, esock_atom_eagain); } else { ERL_NIF_TERM res = esock_make_error_errno(env, saveErrno); @@ -13740,6 +13753,8 @@ ERL_NIF_TERM recv_check_result(ErlNifEnv* env, recv_error_current_reader(env, descP, res); + FREE_BIN(bufP); + return res; } @@ -13768,6 +13783,9 @@ ERL_NIF_TERM recv_check_result(ErlNifEnv* env, recv_update_current_reader(env, descP); + /* This transfers "ownership" of the *allocated* binary to an + * erlang term (no need for an explicit free). + */ data = MKBIN(env, bufP); data = MKSBIN(env, data, 0, read); @@ -13792,6 +13810,9 @@ ERL_NIF_TERM recv_check_result(ErlNifEnv* env, cnt_inc(&descP->readByteCnt, read); + /* This transfers "ownership" of the *allocated* binary to an + * erlang term (no need for an explicit free). + */ data = MKBIN(env, bufP); data = MKSBIN(env, data, 0, read); @@ -15713,6 +15734,7 @@ SocketDescriptor* alloc_descriptor(SOCKET sock, HANDLE event) if ((descP = enif_alloc_resource(sockets, sizeof(SocketDescriptor))) != NULL) { char buf[64]; /* Buffer used for building the mutex name */ + // This needs to be released when the socket is closed! descP->env = enif_alloc_env(); sprintf(buf, "socket[w,%d]", sock); @@ -16870,10 +16892,15 @@ void socket_dtor(ErlNifEnv* env, void* obj) { SocketDescriptor* descP = (SocketDescriptor*) obj; + enif_clear_env(descP->env); + enif_free_env(descP->env); + descP->env = NULL; + MDESTROY(descP->writeMtx); MDESTROY(descP->readMtx); MDESTROY(descP->accMtx); MDESTROY(descP->closeMtx); + } -- cgit v1.2.3