Ensure NIF term creation disallows illegal values

Add a check to enif_make_double to see if its double argument is
infinity or NaN, returning a badarg exception if it is. Change the
erl_nif documentation to specify that enif_make_double returns a
badarg exception if its double argument is either infinity or NaN. Add
tests to nif_SUITE for this change.

Add checks to the enif_make* functions for atoms to prevent the
creation of atoms whose name lengths are greater than the allowed
maximum atom length. The enif_make_atom and enif_make_atom_len
functions now return a badarg exception if the input string is too
long. The enif_make_existing_atom and enif_make_existing_atom_len
functions return false if the input string is too long. Change the
erl_nif documentation to reflect the changes to these functions. Add
tests to nif_SUITE for these changes.

Add a field to ErlNifEnv to track that a NIF has raised an exception
via enif_make_badarg. If a NIF calls enif_make_badarg but then ignores
its return value and instead tries to return a non-exception term as
its return value, the runtime still raises a badarg. This is needed to
prevent enif_make_badarg values resulting from calls to
enif_make_double, enif_make_atom, or enif_make_atom_len from being
erroneously stored within other terms and returned from a NIF. Calling
enif_make_badarg but not returning its return value has been
documented as being illegal ever since enif_make_badarg was added, but
the runtime has not enforced it until now. Add tests for regular and
dirty NIFs to ensure that calls to enif_make_badarg result in badarg
exceptions even if a NIF fails to return the result of
enif_make_badarg as its return value. Add documentation to
enif_make_badarg to specify that calling it raises a badarg even if a
NIF ignores its return value.

6 files changed, 197 insertions, 22 deletions

diff --git a/erts/doc/src/erl_nif.xml b/erts/doc/src/erl_nif.xml
index 3de94be9ff..feba6daaa0 100644
--- a/erts/doc/src/erl_nif.xml
+++ b/erts/doc/src/erl_nif.xml
@@ -898,12 +898,14 @@ typedef enum {
     <func><name><ret>ERL_NIF_TERM</ret><nametext>enif_make_atom(ErlNifEnv* env, const char* name)</nametext></name>
       <fsummary>Create an atom term</fsummary>
       <desc><p>Create an atom term from the null-terminated C-string <c>name</c>
-      with iso-latin-1 encoding.</p></desc>
+      with iso-latin-1 encoding. If the length of <c>name</c> exceeds the maximum length
+      allowed for an atom, <c>enif_make_atom</c> returns a <c>badarg</c> exception.</p></desc>
     </func>
     <func><name><ret>ERL_NIF_TERM</ret><nametext>enif_make_atom_len(ErlNifEnv* env, const char* name, size_t len)</nametext></name>
       <fsummary>Create an atom term</fsummary>
       <desc><p>Create an atom term from the string <c>name</c> with length <c>len</c>.
-      Null-characters are treated as any other characters.</p></desc>
+      Null-characters are treated as any other characters. If <c>len</c> is greater than the maximum length
+      allowed for an atom, <c>enif_make_atom</c> returns a <c>badarg</c> exception.</p></desc>
     </func>
     <func><name><ret>ERL_NIF_TERM</ret><nametext>enif_make_badarg(ErlNifEnv* env)</nametext></name>
       <fsummary>Make a badarg exception.</fsummary>
@@ -911,8 +913,10 @@ typedef enum {
       an associated exception reason in <c>env</c>. If
       <c>enif_make_badarg</c> is called, the term it returns <em>must</em>
       be returned from the function that called it. No other return value
-      is allowed. Also, the term returned from <c>enif_make_badarg</c> may
-      be passed only to
+      is allowed. Once a NIF or any function it calls invokes <c>enif_make_badarg</c>,
+      the runtime ensures that a <c>badarg</c> exception is raised when the NIF
+      returns, even if the NIF attempts to return a non-exception term instead.
+      Also, the term returned from <c>enif_make_badarg</c> may be passed only to
       <seealso marker="#enif_is_exception">enif_is_exception</seealso> and
       not to any other NIF API function.</p></desc>
     </func>
@@ -931,7 +935,9 @@ typedef enum {
     </func>
     <func><name><ret>ERL_NIF_TERM</ret><nametext>enif_make_double(ErlNifEnv* env, double d)</nametext></name>
       <fsummary>Create a floating-point term</fsummary>
-      <desc><p>Create a floating-point term from a <c>double</c>.</p></desc>
+      <desc><p>Create a floating-point term from a <c>double</c>. If the <c>double</c> argument is
+      not finite or is NaN, <c>enif_make_double</c> returns a <c>badarg</c> exception.</p>
+      </desc>
     </func>
     <func><name><ret>int</ret><nametext>enif_make_existing_atom(ErlNifEnv* env, const char* name, ERL_NIF_TERM* atom, ErlNifCharEncoding encode)</nametext></name>
       <fsummary>Create an existing atom term</fsummary>
@@ -939,7 +945,8 @@ typedef enum {
       the null-terminated C-string <c>name</c> with encoding
       <seealso marker="#ErlNifCharEncoding">encode</seealso>. If the atom
       already exists store the term in <c>*atom</c> and return true, otherwise
-      return false.</p></desc>
+      return false. If the length of <c>name</c> exceeds the maximum length
+      allowed for an atom, <c>enif_make_existing_atom</c> returns false.</p></desc>
     </func>
     <func><name><ret>int</ret><nametext>enif_make_existing_atom_len(ErlNifEnv* env, const char* name, size_t len, ERL_NIF_TERM* atom, ErlNifCharEncoding encoding)</nametext></name>
       <fsummary>Create an existing atom term</fsummary>
@@ -947,7 +954,9 @@ typedef enum {
       string <c>name</c> with length <c>len</c> and encoding
       <seealso marker="#ErlNifCharEncoding">encode</seealso>. Null-characters
       are treated as any other characters. If the atom already exists store the term
-      in <c>*atom</c> and return true, otherwise return false.</p></desc>
+      in <c>*atom</c> and return true, otherwise return false. If <c>len</c> is greater
+      than the maximum length allowed for an atom, <c>enif_make_existing_atom_len</c>
+      returns false.</p></desc>
     </func>
     <func><name><ret>ERL_NIF_TERM</ret><nametext>enif_make_int(ErlNifEnv* env, int i)</nametext></name>
       <fsummary>Create an integer term</fsummary>
diff --git a/erts/emulator/beam/beam_emu.c b/erts/emulator/beam/beam_emu.c
index 8bfb7d2ad2..d352528059 100644
--- a/erts/emulator/beam/beam_emu.c
+++ b/erts/emulator/beam/beam_emu.c
@@ -3523,6 +3523,8 @@ get_map_elements_fail:
 		erts_pre_nif(&env, c_p, (struct erl_module_nif*)I[2]);
 		reg[0] = r(0);
 		nif_bif_result = (*fp)(&env, bif_nif_arity, reg);
+		if (env.exception_thrown)
+		    nif_bif_result = THE_NON_VALUE;
 		erts_post_nif(&env);
 	    }
 	    ASSERT(!ERTS_PROC_IS_EXITING(c_p) || is_non_value(nif_bif_result));
diff --git a/erts/emulator/beam/erl_nif.c b/erts/emulator/beam/erl_nif.c
index adc3520ebb..ec82ef251e 100644
--- a/erts/emulator/beam/erl_nif.c
+++ b/erts/emulator/beam/erl_nif.c
@@ -127,6 +127,7 @@ void erts_pre_nif(ErlNifEnv* env, Process* p, struct erl_module_nif* mod_nif)
     env->heap_frag = NULL;
     env->fpe_was_unmasked = erts_block_fpe();
     env->tmp_obj_list = NULL;
+    env->exception_thrown = 0;
 }
 
 static void pre_nif_noproc(ErlNifEnv* env, struct erl_module_nif* mod_nif)
@@ -742,6 +743,7 @@ Eterm enif_make_sub_binary(ErlNifEnv* env, ERL_NIF_TERM bin_term,
 
 Eterm enif_make_badarg(ErlNifEnv* env)
 {
+    env->exception_thrown = 1;
     BIF_ERROR(env->proc, BADARG);
 }
 
@@ -964,7 +966,10 @@ ERL_NIF_TERM enif_make_uint64(ErlNifEnv* env, ErlNifUInt64 i)
 
 ERL_NIF_TERM enif_make_double(ErlNifEnv* env, double d)
 {
-    Eterm* hp = alloc_heap(env,FLOAT_SIZE_OBJECT);
+    Eterm* hp;
+    if (!isfinite(d))
+        return enif_make_badarg(env);
+    hp = alloc_heap(env,FLOAT_SIZE_OBJECT);
     FloatDef f;
     f.fd = d;
     PUT_DOUBLE(f, hp);
@@ -978,6 +983,8 @@ ERL_NIF_TERM enif_make_atom(ErlNifEnv* env, const char* name)
 
 ERL_NIF_TERM enif_make_atom_len(ErlNifEnv* env, const char* name, size_t len)
 {
+    if (len > MAX_ATOM_CHARACTERS)
+        return enif_make_badarg(env);
     return erts_atom_put((byte*)name, len, ERTS_ATOM_ENC_LATIN1, 1);
 }
 
@@ -991,6 +998,8 @@ int enif_make_existing_atom_len(ErlNifEnv* env, const char* name, size_t len,
 				ERL_NIF_TERM* atom, ErlNifCharEncoding encoding)
 {
     ASSERT(encoding == ERL_NIF_LATIN1);
+    if (len > MAX_ATOM_CHARACTERS)
+        return 0;
     return erts_atom_get(name, len, atom, ERTS_ATOM_ENC_LATIN1);
 }
 
@@ -1754,14 +1763,13 @@ execute_dirty_nif(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
     ASSERT(ep);
     if (ep->fp)
 	fp = NULL;
-    if (is_non_value(result)) {
+    if (is_non_value(result) || env->exception_thrown) {
 	if (proc->freason != TRAP) {
-	    ASSERT(proc->freason == BADARG);
 	    return init_nif_sched_data(env, dirty_nif_exception, fp, 0, argc, argv);
 	} else {
 	    if (ep->fp == NULL)
 		restore_nif_mfa(proc, ep, 1);
-	    return result;
+	    return THE_NON_VALUE;
 	}
     }
     else
diff --git a/erts/emulator/beam/global.h b/erts/emulator/beam/global.h
index 32a2dc43e8..d58aecab9b 100644
--- a/erts/emulator/beam/global.h
+++ b/erts/emulator/beam/global.h
@@ -51,6 +51,7 @@ struct enif_environment_t /* ErlNifEnv */
     ErlHeapFragment* heap_frag;
     int fpe_was_unmasked;
     struct enif_tmp_obj_t* tmp_obj_list;
+    int exception_thrown; /* boolean */
 };
 extern void erts_pre_nif(struct enif_environment_t*, Process*,
 			 struct erl_module_nif*);
diff --git a/erts/emulator/test/nif_SUITE.erl b/erts/emulator/test/nif_SUITE.erl
index 4560077a51..c11a6ad7c5 100644
--- a/erts/emulator/test/nif_SUITE.erl
+++ b/erts/emulator/test/nif_SUITE.erl
@@ -39,7 +39,8 @@
 	 get_length/1, make_atom/1, make_string/1, reverse_list_test/1,
 	 otp_9828/1,
 	 otp_9668/1, consume_timeslice/1, dirty_nif/1, dirty_nif_send/1,
-	 dirty_nif_exception/1, nif_schedule/1
+	 dirty_nif_exception/1, nif_schedule/1,
+	 nif_exception/1, nif_nan_and_inf/1, nif_atom_too_long/1
 	]).
 
 -export([many_args_100/100]).
@@ -68,7 +69,8 @@ all() ->
      make_string,reverse_list_test,
      otp_9828,
      otp_9668, consume_timeslice,
-     nif_schedule, dirty_nif, dirty_nif_send, dirty_nif_exception
+     nif_schedule, dirty_nif, dirty_nif_send, dirty_nif_exception,
+     nif_exception, nif_nan_and_inf, nif_atom_too_long
     ].
 
 groups() -> 
@@ -1595,11 +1597,27 @@ dirty_nif_exception(Config) when is_list(Config) ->
 	N when is_integer(N) ->
 	    ensure_lib_loaded(Config),
 	    try
-	        call_dirty_nif_exception(),
+		%% this checks that the expected exception
+		%% occurs when the NIF returns the result
+		%% of enif_make_badarg directly
+	        call_dirty_nif_exception(1),
 	        ?t:fail(expected_badarg)
 	    catch
 	        error:badarg ->
-		    [{?MODULE,call_dirty_nif_exception,[],_}|_] =
+		    [{?MODULE,call_dirty_nif_exception,[1],_}|_] =
+			erlang:get_stacktrace(),
+		    ok
+	    end,
+	    try
+		%% this checks that the expected exception
+		%% occurs when the NIF calls enif_make_badarg
+		%% at some point but then returns a value that
+		%% isn't an exception
+	        call_dirty_nif_exception(0),
+	        ?t:fail(expected_badarg)
+	    catch
+	        error:badarg ->
+		    [{?MODULE,call_dirty_nif_exception,[0],_}|_] =
 			erlang:get_stacktrace(),
 		    ok
 	    end
@@ -1608,6 +1626,57 @@ dirty_nif_exception(Config) when is_list(Config) ->
 	    {skipped,"No dirty scheduler support"}
     end.
 
+nif_exception(Config) when is_list(Config) ->
+    ensure_lib_loaded(Config),
+    try
+	call_nif_exception(),
+	?t:fail(expected_badarg)
+    catch
+	error:badarg ->
+	    ok
+    end.
+
+nif_nan_and_inf(Config) when is_list(Config) ->
+    ensure_lib_loaded(Config),
+    try
+	call_nif_nan_or_inf(nan),
+	?t:fail(expected_badarg)
+    catch
+	error:badarg ->
+	    ok
+    end,
+    try
+	call_nif_nan_or_inf(inf),
+	?t:fail(expected_badarg)
+    catch
+	error:badarg ->
+	    ok
+    end,
+    try
+	call_nif_nan_or_inf(tuple),
+	?t:fail(expected_badarg)
+    catch
+	error:badarg ->
+	    ok
+    end.
+
+nif_atom_too_long(Config) when is_list(Config) ->
+    ensure_lib_loaded(Config),
+    try
+	call_nif_atom_too_long(all),
+	?t:fail(expected_badarg)
+    catch
+	error:badarg ->
+	    ok
+    end,
+    try
+	call_nif_atom_too_long(len),
+	?t:fail(expected_badarg)
+    catch
+	error:badarg ->
+	    ok
+    end.
+
 next_msg(_Pid) ->
     receive
 	M -> M
@@ -1741,8 +1810,11 @@ consume_timeslice_nif(_,_) -> ?nif_stub.
 call_nif_schedule(_,_) -> ?nif_stub.
 call_dirty_nif(_,_,_) -> ?nif_stub.
 send_from_dirty_nif(_) -> ?nif_stub.
-call_dirty_nif_exception() -> ?nif_stub.
+call_dirty_nif_exception(_) -> ?nif_stub.
 call_dirty_nif_zero_args() -> ?nif_stub.
+call_nif_exception() -> ?nif_stub.
+call_nif_nan_or_inf(_) -> ?nif_stub.
+call_nif_atom_too_long(_) -> ?nif_stub.
 
 %% maps
 is_map_nif(_) -> ?nif_stub.
diff --git a/erts/emulator/test/nif_SUITE_data/nif_SUITE.c b/erts/emulator/test/nif_SUITE_data/nif_SUITE.c
index 85544db2ab..d5109f1e58 100644
--- a/erts/emulator/test/nif_SUITE_data/nif_SUITE.c
+++ b/erts/emulator/test/nif_SUITE_data/nif_SUITE.c
@@ -380,7 +380,8 @@ static ERL_NIF_TERM type_test(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[
     ErlNifSInt64 sint64;
     ErlNifUInt64 uint64;
     double d;
-    ERL_NIF_TERM atom, ref1, ref2;
+    ERL_NIF_TERM atom, ref1, ref2, term;
+    size_t len;
 
     sint = INT_MIN;
     do {
@@ -502,6 +503,7 @@ static ERL_NIF_TERM type_test(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[
 	    goto error;
 	}
     }
+
     ref1 = enif_make_ref(env);
     ref2 = enif_make_ref(env);
     if (!enif_is_ref(env,ref1) || !enif_is_ref(env,ref2) 
@@ -1608,16 +1610,26 @@ static ERL_NIF_TERM send_from_dirty_nif(ErlNifEnv* env, int argc, const ERL_NIF_
 static ERL_NIF_TERM call_dirty_nif_exception(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
 {
     switch (argc) {
-    case 0: {
+    case 1: {
 	ERL_NIF_TERM args[255];
 	int i;
-	for (i = 0; i < 255; i++)
+	args[0] = argv[0];
+	for (i = 1; i < 255; i++)
 	    args[i] = enif_make_int(env, i);
 	return enif_schedule_nif(env, "call_dirty_nif_exception", ERL_NIF_DIRTY_JOB_CPU_BOUND,
 				 call_dirty_nif_exception, 255, argv);
     }
-    case 1:
-	return enif_make_badarg(env);
+    case 2: {
+        int return_badarg_directly;
+        enif_get_int(env, argv[0], &return_badarg_directly);
+        assert(return_badarg_directly == 1 || return_badarg_directly == 0);
+        if (return_badarg_directly)
+            return enif_make_badarg(env);
+        else {
+            /* ignore return value */ enif_make_badarg(env);
+            return enif_make_atom(env, "ok");
+        }
+    }
     default:
 	return enif_schedule_nif(env, "call_dirty_nif_exception", ERL_NIF_DIRTY_JOB_CPU_BOUND,
 				 call_dirty_nif_exception, argc-1, argv);
@@ -1637,6 +1649,74 @@ static ERL_NIF_TERM call_dirty_nif_zero_args(ErlNifEnv* env, int argc, const ERL
 }
 #endif
 
+/*
+ * Call enif_make_badarg, but don't return its return value. Instead,
+ * return ok.  Result should still be a badarg exception for the erlang
+ * caller.
+ */
+static ERL_NIF_TERM call_nif_exception(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
+{
+    /* ignore return value */ enif_make_badarg(env);
+    return enif_make_atom(env, "ok");
+}
+
+static ERL_NIF_TERM call_nif_nan_or_inf(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
+{
+    double val;
+    char arg[6];
+    ERL_NIF_TERM res;
+
+    assert(argc == 1);
+    enif_get_atom(env, argv[0], arg, sizeof arg, ERL_NIF_LATIN1);
+    if (strcmp(arg, "nan") == 0) {
+        /* Verify that enif_make_double raises a badarg for NaN */
+#ifdef NAN
+        val = NAN;
+#else
+        val = 0.0/0.0;
+#endif
+    } else {
+        /* Verify that enif_make_double raises a badarg for NaN and infinity */
+#ifdef INFINITY
+        val = INFINITY;
+#else
+        val = 1.0/0.0;
+#endif
+    }
+    res = enif_make_double(env, val);
+    assert(enif_is_exception(env, res));
+    if (strcmp(arg, "tuple") == 0) {
+        return enif_make_tuple2(env, argv[0], res);
+    } else {
+        return res;
+    }
+}
+
+static ERL_NIF_TERM call_nif_atom_too_long(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
+{
+    char str[257];
+    char arg[4];
+    size_t len;
+    int i;
+    ERL_NIF_TERM res;
+
+    assert(argc == 1);
+    enif_get_atom(env, argv[0], arg, sizeof arg, ERL_NIF_LATIN1);
+    /* Verify that creating an atom from a string that's too long results in a badarg */
+    for (i = 0; i < sizeof str; ++i) {
+        str[i] = 'a';
+    }
+    str[256] = '\0';
+    if (strcmp(arg, "len") == 0) {
+        len = strlen(str);
+        res = enif_make_atom_len(env, str, len);
+    } else {
+        res = enif_make_atom(env, str);
+    }
+    assert(enif_is_exception(env, res));
+    return res;
+}
+
 static ERL_NIF_TERM is_map_nif(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
 {
     return enif_make_int(env, enif_is_map(env,argv[0]));
@@ -1818,9 +1898,12 @@ static ErlNifFunc nif_funcs[] =
 #ifdef ERL_NIF_DIRTY_SCHEDULER_SUPPORT
     {"call_dirty_nif", 3, call_dirty_nif},
     {"send_from_dirty_nif", 1, send_from_dirty_nif, ERL_NIF_DIRTY_JOB_CPU_BOUND},
-    {"call_dirty_nif_exception", 0, call_dirty_nif_exception, ERL_NIF_DIRTY_JOB_IO_BOUND},
+    {"call_dirty_nif_exception", 1, call_dirty_nif_exception, ERL_NIF_DIRTY_JOB_IO_BOUND},
     {"call_dirty_nif_zero_args", 0, call_dirty_nif_zero_args, ERL_NIF_DIRTY_JOB_CPU_BOUND},
 #endif
+    {"call_nif_exception", 0, call_nif_exception},
+    {"call_nif_nan_or_inf", 1, call_nif_nan_or_inf},
+    {"call_nif_atom_too_long", 1, call_nif_atom_too_long},
     {"is_map_nif", 1, is_map_nif},
     {"get_map_size_nif", 1, get_map_size_nif},
     {"make_new_map_nif", 0, make_new_map_nif},