erts: Fix NIF bug when load/upgrade fails after enif_open_resource_type

..has been successfully called.

Opened resource types (created or taken-over) were left "hanging"
leading both to memory leakage and other more strange and serious behavior.

Now a proper rollback is done.

4 files changed, 257 insertions, 42 deletions

diff --git a/erts/emulator/beam/erl_nif.c b/erts/emulator/beam/erl_nif.c
index dc285b3cf7..b2fd3dcd24 100644
--- a/erts/emulator/beam/erl_nif.c
+++ b/erts/emulator/beam/erl_nif.c
@@ -1,7 +1,7 @@
 /*
  * %CopyrightBegin%
  *
- * Copyright Ericsson AB 2009-2013. All Rights Reserved.
+ * Copyright Ericsson AB 2009-2014. All Rights Reserved.
  *
  * The contents of this file are subject to the Erlang Public License,
  * Version 1.1, (the "License"); you may not use this file except in
@@ -1235,6 +1235,19 @@ static void steal_resource_type(ErlNifResourceType* type)
     }
 }
 
+/* The opened_rt_list is used by enif_open_resource_type()
+ * in order to rollback "creates" and "take-overs" in case the load fails.
+ */
+struct opened_resource_type
+{
+    struct opened_resource_type* next;
+
+    ErlNifResourceFlags op;
+    ErlNifResourceType* type;
+    ErlNifResourceDtor* new_dtor;
+};
+static struct opened_resource_type* opened_rt_list = NULL;
+
 ErlNifResourceType*
 enif_open_resource_type(ErlNifEnv* env,
 			const char* module_str, 
@@ -1256,22 +1269,21 @@ enif_open_resource_type(ErlNifEnv* env,
     if (type == NULL) {
 	if (flags & ERL_NIF_RT_CREATE) {
 	    type = erts_alloc(ERTS_ALC_T_NIF,
-			      sizeof(struct enif_resource_type_t)); 
-	    type->dtor = dtor;
+			      sizeof(struct enif_resource_type_t));
 	    type->module = module_am;
 	    type->name = name_am;
 	    erts_refc_init(&type->refc, 1);
-	    type->owner = env->mod_nif;
-	    type->prev = &resource_type_list;
-	    type->next = resource_type_list.next;
-	    type->next->prev = type;
-	    type->prev->next = type;
 	    op = ERL_NIF_RT_CREATE;
+	#ifdef DEBUG
+	    type->dtor = (void*)1;
+	    type->owner = (void*)2;
+	    type->prev = (void*)3;
+	    type->next = (void*)4;
+	#endif
 	}
     }
     else {
-	if (flags & ERL_NIF_RT_TAKEOVER) {	
-	    steal_resource_type(type);
+	if (flags & ERL_NIF_RT_TAKEOVER) {
 	    op = ERL_NIF_RT_TAKEOVER;
 	}
 	else {
@@ -1279,12 +1291,13 @@ enif_open_resource_type(ErlNifEnv* env,
 	}
     }
     if (type != NULL) {
-	type->owner = env->mod_nif;
-	type->dtor = dtor;
-	if (type->dtor != NULL) {
-	    erts_refc_inc(&type->owner->rt_dtor_cnt, 1);
-	}
-	erts_refc_inc(&type->owner->rt_cnt, 1);    
+	struct opened_resource_type* ort = erts_alloc(ERTS_ALC_T_TMP,
+						sizeof(struct opened_resource_type));
+	ort->op = op;
+	ort->type = type;
+	ort->new_dtor = dtor;
+	ort->next = opened_rt_list;
+	opened_rt_list = ort;
     }
     if (tried != NULL) {
 	*tried = op;
@@ -1292,6 +1305,51 @@ enif_open_resource_type(ErlNifEnv* env,
     return type;
 }
 
+static void commit_opened_resource_types(struct erl_module_nif* lib)
+{
+    while (opened_rt_list) {
+	struct opened_resource_type* ort = opened_rt_list;
+
+	ErlNifResourceType* type = ort->type;
+
+	if (ort->op == ERL_NIF_RT_CREATE) {
+	    type->prev = &resource_type_list;
+	    type->next = resource_type_list.next;
+	    type->next->prev = type;
+	    type->prev->next = type;
+	}
+	else { /* ERL_NIF_RT_TAKEOVER */
+	    steal_resource_type(type);
+	}
+
+	type->owner = lib;
+	type->dtor = ort->new_dtor;
+
+	if (type->dtor != NULL) {
+	    erts_refc_inc(&lib->rt_dtor_cnt, 1);
+	}
+	erts_refc_inc(&lib->rt_cnt, 1);
+
+	opened_rt_list = ort->next;
+	erts_free(ERTS_ALC_T_TMP, ort);
+    }
+}
+
+static void rollback_opened_resource_types(void)
+{
+    while (opened_rt_list) {
+	struct opened_resource_type* ort = opened_rt_list;
+
+	if (ort->op == ERL_NIF_RT_CREATE) {
+	    erts_free(ERTS_ALC_T_NIF, ort->type);
+	}
+
+	opened_rt_list = ort->next;
+	erts_free(ERTS_ALC_T_TMP, ort);
+    }
+}
+
+
 static void nif_resource_dtor(Binary* bin)
 {
     ErlNifResource* resource = (ErlNifResource*) ERTS_MAGIC_BIN_DATA(bin);
@@ -1317,6 +1375,8 @@ void* enif_alloc_resource(ErlNifResourceType* type, size_t size)
 {
     Binary* bin = erts_create_magic_binary(SIZEOF_ErlNifResource(size), &nif_resource_dtor);
     ErlNifResource* resource = ERTS_MAGIC_BIN_DATA(bin);
+
+    ASSERT(type->owner && type->next && type->prev); /* not allowed in load/upgrade */
     resource->type = type;
     erts_refc_inc(&bin->refc, 1);
 #ifdef DEBUG
@@ -1696,9 +1756,15 @@ BIF_RETTYPE load_nif_2(BIF_ALIST_2)
     lib->entry = entry;
     erts_refc_init(&lib->rt_cnt, 0);
     erts_refc_init(&lib->rt_dtor_cnt, 0);
+    ASSERT(opened_rt_list == NULL);
     lib->mod = mod;
     env.mod_nif = lib;
-    if (mod->curr.nif != NULL) { /* Reload */
+    if (mod->curr.nif != NULL) { /*************** Reload ******************/
+	/*
+	 * Repeated load_nif calls from same Erlang module instance ("reload")
+	 * is deprecated and was only ment as a development feature not to
+	 * be used in production systems. (See warning below)
+	 */
 	int k;
         lib->priv_data = mod->curr.nif->priv_data;
 
@@ -1730,6 +1796,7 @@ BIF_RETTYPE load_nif_2(BIF_ALIST_2)
 	    ret = load_nif_error(BIF_P, reload, "Library reload-call unsuccessful.");
 	}
 	else {
+	    commit_opened_resource_types(lib);
 	    mod->curr.nif->entry = NULL; /* to prevent 'unload' callback */
 	    erts_unload_nif(mod->curr.nif);
 	    reload_warning = 1;
@@ -1737,7 +1804,7 @@ BIF_RETTYPE load_nif_2(BIF_ALIST_2)
     }
     else {
 	lib->priv_data = NULL;
-	if (mod->old.nif != NULL) { /* Upgrade */
+	if (mod->old.nif != NULL) { /**************** Upgrade ***************/
 	    void* prev_old_data = mod->old.nif->priv_data;
 	    if (entry->upgrade == NULL) {
 		ret = load_nif_error(BIF_P, upgrade, "Upgrade not supported by this NIF library.");
@@ -1750,17 +1817,18 @@ BIF_RETTYPE load_nif_2(BIF_ALIST_2)
 		mod->old.nif->priv_data = prev_old_data;
 		ret = load_nif_error(BIF_P, upgrade, "Library upgrade-call unsuccessful.");
 	    }
-	    /*else if (mod->old_nif->priv_data != prev_old_data) {
-		refresh_cached_nif_data(mod->old_code, mod->old_nif);
-	    }*/
+	    else
+		commit_opened_resource_types(lib);
 	}
-	else if (entry->load != NULL) { /* Initial load */
+	else if (entry->load != NULL) { /********* Initial load ***********/
 	    erts_pre_nif(&env, BIF_P, lib);
 	    veto = entry->load(&env, &lib->priv_data, BIF_ARG_2);
 	    erts_post_nif(&env);
 	    if (veto) {
 		ret = load_nif_error(BIF_P, "load", "Library load-call unsuccessful.");
 	    }
+	    else
+		commit_opened_resource_types(lib);
 	}
     }
     if (ret == am_ok) {
@@ -1789,6 +1857,7 @@ BIF_RETTYPE load_nif_2(BIF_ALIST_2)
     }
     else {
     error:
+	rollback_opened_resource_types();
 	ASSERT(ret != am_ok);
         if (lib != NULL) {
 	    erts_free(ERTS_ALC_T_NIF, lib);
diff --git a/erts/emulator/test/nif_SUITE.erl b/erts/emulator/test/nif_SUITE.erl
index 9a70e8646a..c88fa4b228 100644
--- a/erts/emulator/test/nif_SUITE.erl
+++ b/erts/emulator/test/nif_SUITE.erl
@@ -1,7 +1,7 @@
 %%
 %% %CopyrightBegin%
 %%
-%% Copyright Ericsson AB 2010-2013. All Rights Reserved.
+%% Copyright Ericsson AB 2010-2014. All Rights Reserved.
 %%
 %% The contents of this file are subject to the Erlang Public License,
 %% Version 1.1, (the "License"); you may not use this file except in
@@ -801,6 +801,138 @@ resource_takeover(Config) when is_list(Config) ->
     ?line ok = forget_resource(AN4),
     ?line [] = nif_mod_call_history(),
 
+
+    %%
+    %% Test rollback after failed upgrade of same lib-version
+    %%
+
+    {A5,BinA5} = make_resource(2, Holder, "A5"),
+    {NA5,BinNA5} = make_resource(0, Holder, "NA5"),
+    {AN5,_BinAN5} = make_resource(1, Holder, "AN5"),
+
+    {A6,BinA6} = make_resource(2, Holder, "A6"),
+    {NA6,BinNA6} = make_resource(0, Holder, "NA6"),
+    {AN6,_BinAN6} = make_resource(1, Holder, "AN6"),
+
+    {module,nif_mod} = erlang:load_module(nif_mod,ModBin),
+    undefined = nif_mod:lib_version(),
+    {error,{upgrade,_}} =
+	nif_mod:load_nif_lib(Config, 1,
+			     [{resource_type, 4, ?RT_TAKEOVER, "resource_type_A",resource_dtor_B,
+			       ?RT_TAKEOVER},
+			      {resource_type, 4, ?RT_TAKEOVER bor ?RT_CREATE, "resource_type_null_A",null,
+			       ?RT_TAKEOVER},
+			      {resource_type, 4, ?RT_TAKEOVER, "resource_type_A_null",resource_dtor_A,
+			       ?RT_TAKEOVER},
+			      {resource_type, 4, ?RT_CREATE, "Mr Pink", resource_dtor_A,
+			       ?RT_CREATE},
+
+			      {return, 1}  % FAIL
+			     ]),
+
+    undefined = nif_mod:lib_version(),
+    [{upgrade,1,5,105}] = nif_mod_call_history(),
+
+    %% Make sure dtor was not changed (from A to B)
+    ok = forget_resource(A5),
+    [{{resource_dtor_A_v1,BinA5},1,6,106}] = nif_mod_call_history(),
+
+    %% Make sure dtor was not nullified (from A to null)
+    ok = forget_resource(NA5),
+    [{{resource_dtor_A_v1,BinNA5},1,7,107}] = nif_mod_call_history(),
+
+    %% Make sure dtor was not added (from null to A)
+    ok = forget_resource(AN5),
+    [] = nif_mod_call_history(),
+
+    %%
+    %% Test rollback after failed upgrade of other lib-version
+    %%
+
+    {error,{upgrade,_}} =
+	nif_mod:load_nif_lib(Config, 2,
+			     [{resource_type, 4, ?RT_TAKEOVER, "resource_type_A",resource_dtor_B,
+			       ?RT_TAKEOVER},
+			      {resource_type, 4, ?RT_TAKEOVER bor ?RT_CREATE, "resource_type_null_A",null,
+			       ?RT_TAKEOVER},
+			      {resource_type, 4, ?RT_TAKEOVER, "resource_type_A_null",resource_dtor_A,
+			       ?RT_TAKEOVER},
+			      {resource_type, null, ?RT_TAKEOVER, "Mr Pink", resource_dtor_A,
+			       ?RT_TAKEOVER},
+			      {resource_type, 4, ?RT_CREATE, "Mr Pink", resource_dtor_A,
+			       ?RT_CREATE},
+
+			      {return, 1}  % FAIL
+			     ]),
+
+    undefined = nif_mod:lib_version(),
+    [{upgrade,2,_,_}] = nif_mod_call_history(),
+
+    %% Make sure dtor was not changed (from A to B)
+    ok = forget_resource(A6),
+    [{{resource_dtor_A_v1,BinA6},1,_,_}] = nif_mod_call_history(),
+
+    %% Make sure dtor was not nullified (from A to null)
+    ok = forget_resource(NA6),
+    [{{resource_dtor_A_v1,BinNA6},1,_,_}] = nif_mod_call_history(),
+
+    %% Make sure dtor was not added (from null to A)
+    ok = forget_resource(AN6),
+    [] = nif_mod_call_history(),
+
+    %%
+    %% Test rolback after failed initial load
+    %%
+    false = code:purge(nif_mod),
+    [{unload,1,_,_}] = nif_mod_call_history(),
+    true = code:delete(nif_mod),
+    false = code:purge(nif_mod),
+    [] = nif_mod_call_history(),
+
+
+    {module,nif_mod} = erlang:load_module(nif_mod,ModBin),
+    undefined = nif_mod:lib_version(),
+    {error,{load,_}} =
+	nif_mod:load_nif_lib(Config, 1,
+			     [{resource_type, null, ?RT_TAKEOVER, "resource_type_A",resource_dtor_A,
+			       ?RT_TAKEOVER},
+			      {resource_type, 4, ?RT_TAKEOVER bor ?RT_CREATE, "resource_type_null_A",null,
+			       ?RT_CREATE},
+			      {resource_type, 4, ?RT_CREATE, "resource_type_A_null",resource_dtor_A,
+			       ?RT_CREATE},
+			      {resource_type, 4, ?RT_CREATE, "Mr Pink", resource_dtor_A,
+			       ?RT_CREATE},
+
+			      {return, 1}  % FAIL
+			     ]),
+
+    undefined = nif_mod:lib_version(),
+    ok = nif_mod:load_nif_lib(Config, 1,
+			      [{resource_type, null, ?RT_TAKEOVER, "resource_type_A",resource_dtor_A,
+				?RT_TAKEOVER},
+			       {resource_type, 0, ?RT_TAKEOVER bor ?RT_CREATE, "resource_type_null_A",
+				resource_dtor_A, ?RT_CREATE},
+
+			       {resource_type, 1, ?RT_CREATE, "resource_type_A_null", null,
+				?RT_CREATE},
+			       {resource_type, null, ?RT_TAKEOVER, "Mr Pink", resource_dtor_A,
+				?RT_TAKEOVER},
+
+			       {return, 0}  % SUCCESS
+			      ]),
+
+    ?line hold_nif_mod_priv_data(nif_mod:get_priv_data_ptr()),
+    ?line [{load,1,1,101},{get_priv_data_ptr,1,2,102}] = nif_mod_call_history(),
+
+    {NA7,BinNA7} = make_resource(0, Holder, "NA7"),
+    {AN7,BinAN7} = make_resource(1, Holder, "AN7"),
+
+    ok = forget_resource(NA7),
+    [{{resource_dtor_A_v1,BinNA7},1,_,_}] = nif_mod_call_history(),
+
+    ok = forget_resource(AN7),
+    [] = nif_mod_call_history(),
+
     ?line true = lists:member(?MODULE, erlang:system_info(taints)),
     ?line true = lists:member(nif_mod, erlang:system_info(taints)),
     ?line verify_tmpmem(TmpMem),    
@@ -1343,7 +1475,7 @@ consume_timeslice(Config) when is_list(Config) ->
 
     ok.
 
-next_msg(Pid) ->
+next_msg(_Pid) ->
     receive
 	M -> M
     after 100 ->
diff --git a/erts/emulator/test/nif_SUITE_data/nif_SUITE.c b/erts/emulator/test/nif_SUITE_data/nif_SUITE.c
index 0c4a9f7e5c..cdc64b89b2 100644
--- a/erts/emulator/test/nif_SUITE_data/nif_SUITE.c
+++ b/erts/emulator/test/nif_SUITE_data/nif_SUITE.c
@@ -1,7 +1,7 @@
 /*
  * %CopyrightBegin%
  *
- * Copyright Ericsson AB 2009-2013. All Rights Reserved.
+ * Copyright Ericsson AB 2009-2014. All Rights Reserved.
  *
  * The contents of this file are subject to the Erlang Public License,
  * Version 1.1, (the "License"); you may not use this file except in
@@ -1477,7 +1477,6 @@ static ERL_NIF_TERM consume_timeslice_nif(ErlNifEnv* env, int argc, const ERL_NI
 {
     int percent;
     char atom[10];
-    int do_repeat;
 
     if (!enif_get_int(env, argv[0], &percent) ||
 	!enif_get_atom(env, argv[1], atom, sizeof(atom), ERL_NIF_LATIN1)) {
diff --git a/erts/emulator/test/nif_SUITE_data/nif_mod.c b/erts/emulator/test/nif_SUITE_data/nif_mod.c
index e32d10057c..aed8524635 100644
--- a/erts/emulator/test/nif_SUITE_data/nif_mod.c
+++ b/erts/emulator/test/nif_SUITE_data/nif_mod.c
@@ -1,7 +1,7 @@
 /*
  * %CopyrightBegin%
  *
- * Copyright Ericsson AB 2009-2010. All Rights Reserved.
+ * Copyright Ericsson AB 2009-2014. All Rights Reserved.
  *
  * The contents of this file are subject to the Erlang Public License,
  * Version 1.1, (the "License"); you may not use this file except in
@@ -41,6 +41,7 @@ static ERL_NIF_TERM am_null;
 static ERL_NIF_TERM am_resource_type;
 static ERL_NIF_TERM am_resource_dtor_A;
 static ERL_NIF_TERM am_resource_dtor_B;
+static ERL_NIF_TERM am_return;
 
 static NifModPrivData* priv_data(ErlNifEnv* env)
 {
@@ -54,6 +55,7 @@ static void init(ErlNifEnv* env)
     am_resource_type = enif_make_atom(env, "resource_type");
     am_resource_dtor_A = enif_make_atom(env, "resource_dtor_A");
     am_resource_dtor_B = enif_make_atom(env, "resource_dtor_B");
+    am_return = enif_make_atom(env, "return");
 }
 
 static void add_call_with_arg(ErlNifEnv* env, NifModPrivData* data, const char* func_name,
@@ -105,19 +107,15 @@ static void resource_dtor_B(ErlNifEnv* env, void* a)
 }
 
 /* {resource_type, Ix|null, ErlNifResourceFlags in, "TypeName", dtor(A|B|null), ErlNifResourceFlags out}*/
-static void open_resource_type(ErlNifEnv* env, ERL_NIF_TERM op_tpl)
+static void open_resource_type(ErlNifEnv* env, const ERL_NIF_TERM* arr)
 {
     NifModPrivData* data = priv_data(env);
-    const ERL_NIF_TERM* arr;
-    int arity;
     char rt_name[30];
     union { ErlNifResourceFlags e; int i; } flags, exp_res, got_res;
     unsigned ix;
     ErlNifResourceDtor* dtor;
     ErlNifResourceType* got_ptr;
 
-    CHECK(enif_get_tuple(env, op_tpl, &arity, &arr));
-    CHECK(arity == 6);
     CHECK(enif_is_identical(arr[0], am_resource_type));
     CHECK(enif_get_int(env, arr[2], &flags.i));
     CHECK(enif_get_string(env, arr[3], rt_name, sizeof(rt_name), ERL_NIF_LATIN1) > 0);
@@ -147,18 +145,32 @@ static void open_resource_type(ErlNifEnv* env, ERL_NIF_TERM op_tpl)
     CHECK(got_res.e == exp_res.e);
 }
 
-static void do_load_info(ErlNifEnv* env, ERL_NIF_TERM load_info)
+static void do_load_info(ErlNifEnv* env, ERL_NIF_TERM load_info, int* retvalp)
 {
     NifModPrivData* data = priv_data(env);
     ERL_NIF_TERM head, tail;
     unsigned ix;
+
     for (ix=0; ix<RT_MAX; ix++) {
 	data->rt_arr[ix] = NULL;
     }
     for (head = load_info; enif_get_list_cell(env, head, &head, &tail);
 	  head = tail) {
-
-	open_resource_type(env, head);	
+	const ERL_NIF_TERM* arr;
+	int arity;
+
+	CHECK(enif_get_tuple(env, head, &arity, &arr));
+	switch (arity) {
+	case 6:
+	    open_resource_type(env, arr);
+	    break;
+	case 2:
+	    CHECK(arr[0] == am_return);
+	    CHECK(enif_get_int(env, arr[1], retvalp));
+	    break;
+	default:
+	    CHECK(0);
+	}
     }    
     CHECK(enif_is_empty_list(env, head));
 }
@@ -166,6 +178,7 @@ static void do_load_info(ErlNifEnv* env, ERL_NIF_TERM load_info)
 static int load(ErlNifEnv* env, void** priv, ERL_NIF_TERM load_info)
 {
     NifModPrivData* data;
+    int retval = 0;
 
     init(env);
     data = (NifModPrivData*) enif_alloc(sizeof(NifModPrivData));
@@ -177,38 +190,40 @@ static int load(ErlNifEnv* env, void** priv, ERL_NIF_TERM load_info)
 
     add_call(env, data, "load");
 
-    do_load_info(env, load_info);
+    do_load_info(env, load_info, &retval);
     data->calls = 0;    
-    return 0;
+    return retval;
 }
 
 static int reload(ErlNifEnv* env, void** priv, ERL_NIF_TERM load_info)
 {
     NifModPrivData* data = (NifModPrivData*) *priv;
+    int retval = 0;
     init(env);
     add_call(env, data, "reload");
     
-    do_load_info(env, load_info);
-    return 0;
+    do_load_info(env, load_info, &retval);
+    return retval;
 }
 
 static int upgrade(ErlNifEnv* env, void** priv, void** old_priv_data, ERL_NIF_TERM load_info)
 {
     NifModPrivData* data = (NifModPrivData*) *old_priv_data;
+    int retval = 0;
     init(env);
     add_call(env, data, "upgrade");
     data->ref_cnt++;
 
     *priv = *old_priv_data;    
-    do_load_info(env, load_info);
+    do_load_info(env, load_info, &retval);
     
-    return 0;
+    return retval;
 }
 
 static void unload(ErlNifEnv* env, void* priv)
 {
     NifModPrivData* data = (NifModPrivData*) priv;
-    int is_last;
+
     add_call(env, data, "unload");
     NifModPrivData_release(data);
 }