Skip to content

Commit f12f0b8

Browse files
committed
fix(nif): low-severity robustness hardening
Guard make_py_error against a NULL message/type when a Python exception's text isn't UTF-8-encodable (and against a NULL exception type). Reject embedded NULs in binary_to_string so a module/func/attr/code name can't be silently truncated at the NUL. Release the leaked split() method in ReactorBuffer_split. Drop a stray debug fprintf on the normal worker send path. Document that the worker-pool init/shutdown is gen_server serialized (no lock needed). Adds py_SUITE:test_embedded_nul_name_rejected.
1 parent d5319a7 commit f12f0b8

6 files changed

Lines changed: 53 additions & 9 deletions

File tree

CHANGELOG.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,15 @@
22

33
## Unreleased
44

5+
### Fixed
6+
7+
- **NIF robustness hardening** - `make_py_error` no longer passes a NULL message/type
8+
to `enif_make_string`/`enif_make_atom` when a Python exception's text isn't
9+
UTF-8-encodable; `binary_to_string` rejects names/code containing an embedded NUL
10+
(which would silently truncate a module/function/attr/code string) rather than
11+
truncating; a leaked `split` method object in the reactor buffer is released; and a
12+
stray debug `fprintf` on the normal worker send path is removed.
13+
514
### Security
615

716
- **No shell for venv/installer commands** - `py:ensure_venv` and dependency

c_src/py_convert.c

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -825,13 +825,22 @@ static ERL_NIF_TERM make_py_error(ErlNifEnv *env) {
825825
enif_make_tuple2(env, ATOM_STOP_ITERATION, ATOM_NONE));
826826
}
827827

828-
/* Get exception message */
828+
/* Get exception message. PyUnicode_AsUTF8 can return NULL (e.g. a str with
829+
* lone surrogates); never pass NULL to enif_make_string. */
829830
PyObject *str = PyObject_Str(value);
830-
const char *err_msg = str ? PyUnicode_AsUTF8(str) : "unknown";
831+
const char *err_msg = (str != NULL) ? PyUnicode_AsUTF8(str) : NULL;
832+
if (err_msg == NULL) {
833+
PyErr_Clear();
834+
err_msg = "unknown";
835+
}
831836

832-
/* Get exception type name */
833-
PyObject *type_name = PyObject_GetAttrString(type, "__name__");
834-
const char *type_str = type_name ? PyUnicode_AsUTF8(type_name) : "Exception";
837+
/* Get exception type name (type itself may be NULL after PyErr_Fetch). */
838+
PyObject *type_name = (type != NULL) ? PyObject_GetAttrString(type, "__name__") : NULL;
839+
const char *type_str = (type_name != NULL) ? PyUnicode_AsUTF8(type_name) : NULL;
840+
if (type_str == NULL) {
841+
PyErr_Clear();
842+
type_str = "Exception";
843+
}
835844

836845
/* Build error tuple: {error, {TypeAtom, MessageString}} */
837846
ERL_NIF_TERM error_tuple = enif_make_tuple2(env,

c_src/py_nif.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1674,6 +1674,12 @@ static ERL_NIF_TERM make_py_error(ErlNifEnv *env);
16741674
* @warning Caller must call enif_free() on the returned string
16751675
*/
16761676
static char *binary_to_string(const ErlNifBinary *bin) {
1677+
/* Reject embedded NULs: the result is used as a C string, so a NUL would
1678+
* silently truncate a module/func/attr/code name (a name-smuggling vector).
1679+
* Callers already treat a NULL return as an error. */
1680+
if (bin->size > 0 && memchr(bin->data, '\0', bin->size) != NULL) {
1681+
return NULL;
1682+
}
16771683
char *str = enif_alloc(bin->size + 1);
16781684
if (str != NULL) {
16791685
memcpy(str, bin->data, bin->size);

c_src/py_reactor_buffer.c

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -620,8 +620,13 @@ static PyObject *ReactorBuffer_split(ReactorBufferObject *self, PyObject *args,
620620
return NULL;
621621
}
622622

623-
PyObject *result = PyObject_Call(
624-
PyObject_GetAttrString(bytes_obj, "split"), args, kwargs);
623+
PyObject *split_meth = PyObject_GetAttrString(bytes_obj, "split");
624+
if (split_meth == NULL) {
625+
Py_DECREF(bytes_obj);
626+
return NULL;
627+
}
628+
PyObject *result = PyObject_Call(split_meth, args, kwargs);
629+
Py_DECREF(split_meth);
625630
Py_DECREF(bytes_obj);
626631
return result;
627632
}

c_src/py_worker_pool.c

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -219,9 +219,9 @@ static void py_pool_send_response(py_pool_request_t *req, ERL_NIF_TERM result) {
219219
* Set to NULL to prevent double-free in py_pool_request_free. */
220220
req->msg_env = NULL;
221221
} else {
222+
/* enif_send fails normally when the caller has already died; the
223+
* g_responses_failed counter records it (no stderr spam). */
222224
atomic_fetch_add(&g_responses_failed, 1);
223-
fprintf(stderr, "[DEBUG] enif_send FAILED for req_id=%llu\n",
224-
(unsigned long long)req->request_id);
225225
/* On failure, msg_env is still valid and will be freed in request_free */
226226
}
227227
}
@@ -579,6 +579,9 @@ static void *py_pool_worker_thread(void *arg) {
579579
* ============================================================================ */
580580

581581
static int py_pool_init(int num_workers) {
582+
/* Init/shutdown are serialized by the single Erlang gen_server that owns the
583+
* pool, so this check-then-init runs without a concurrent caller and needs no
584+
* extra lock. */
582585
if (g_pool.initialized) {
583586
return 0; /* Already initialized */
584587
}

test/py_SUITE.erl

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
test_type_conversions/1,
2323
test_nested_types/1,
2424
test_conversion_depth_guard/1,
25+
test_embedded_nul_name_rejected/1,
2526
test_timeout/1,
2627
test_special_floats/1,
2728
test_streaming/1,
@@ -84,6 +85,7 @@ all() ->
8485
test_type_conversions,
8586
test_nested_types,
8687
test_conversion_depth_guard,
88+
test_embedded_nul_name_rejected,
8789
test_timeout,
8890
test_special_floats,
8991
test_streaming,
@@ -326,6 +328,16 @@ test_conversion_depth_guard(_Config) ->
326328
{ok, 4.0} = py:call(math, sqrt, [16]),
327329
ok.
328330

331+
%% @doc A code string with an embedded NUL must be rejected, not silently
332+
%% truncated at the NUL (which would run something other than intended). Exercises
333+
%% the binary_to_string NUL guard shared by all name/code decoding.
334+
test_embedded_nul_name_rejected(_Config) ->
335+
{error, _} = py:eval(<<"1", 0, "+1">>),
336+
{error, _} = py:exec(<<"x = 1", 0, "; y = 2">>),
337+
%% Node still alive and clean code still works.
338+
{ok, 2} = py:eval(<<"1+1">>),
339+
ok.
340+
329341
test_timeout(_Config) ->
330342
%% Test that timeout works - use time.sleep which guarantees delay
331343
%% time.sleep(1) will definitely exceed 100ms timeout

0 commit comments

Comments
 (0)