Address review comments for worker sandbox PR

- Use atomic_init() for proper C11 atomic initialization
- Add mutex lock in sandbox_policy_set_enabled for thread safety
- Fix memory leaks in import whitelist allocation (cleanup on failure)
- Fix memory leak in sandbox_policy_update when parse fails
- Always sync allow_imports so omitting it clears existing whitelist
- Preserve preset flags when explicit block list provided (use |= semantics)
- Remove unused audit_handler field from sandbox_policy_t
- Add os.fork and os.posix_spawn to subprocess blocking docs
- Rename test functions to match their actual behavior
- Add temp file cleanup in test_sandbox_set_policy

Author: benoitc

c_src/py_sandbox.c

@@ -399,7 +399,7 @@ sandbox_policy_t *sandbox_policy_new(void) {
     memset(policy, 0, sizeof(sandbox_policy_t));
     pthread_mutex_init(&policy->mutex, NULL);
     policy->enabled = false;
-    policy->block_flags = 0;
+    atomic_init(&policy->block_flags, 0);
     policy->allowed_imports = NULL;
     policy->allowed_imports_count = 0;
     policy->log_events = false;
@@ -477,13 +477,15 @@ int parse_sandbox_options(ErlNifEnv *env, ERL_NIF_TERM opts, sandbox_policy_t *p
         }
     }
 
-    /* Check for block list */
+    /* Check for block list
+     * Note: An explicit block list is combined with any preset flags using OR.
+     * This allows extending a preset with additional blocked operations. */
     key = enif_make_atom(env, "block");
     if (enif_get_map_value(env, opts, key, &value)) {
         unsigned int list_len;
         if (enif_get_list_length(env, value, &list_len)) {
             ERL_NIF_TERM head, tail = value;
-            policy->block_flags = 0;
+            /* Don't reset block_flags - preserve preset flags and merge with block list */
             policy->enabled = true;  /* Enable if block list provided */
 
             while (enif_get_list_cell(env, tail, &head, &tail)) {
@@ -525,20 +527,36 @@ int parse_sandbox_options(ErlNifEnv *env, ERL_NIF_TERM opts, sandbox_policy_t *p
                 ErlNifBinary bin;
                 if (enif_inspect_binary(env, head, &bin)) {
                     char *import_name = enif_alloc(bin.size + 1);
-                    if (import_name != NULL) {
-                        memcpy(import_name, bin.data, bin.size);
-                        import_name[bin.size] = '\0';
-                        policy->allowed_imports[policy->allowed_imports_count++] = import_name;
+                    if (import_name == NULL) {
+                        /* Cleanup previously allocated imports on failure */
+                        for (size_t i = 0; i < policy->allowed_imports_count; i++) {
+                            enif_free(policy->allowed_imports[i]);
+                        }
+                        enif_free(policy->allowed_imports);
+                        policy->allowed_imports = NULL;
+                        policy->allowed_imports_count = 0;
+                        return -1;
                     }
+                    memcpy(import_name, bin.data, bin.size);
+                    import_name[bin.size] = '\0';
+                    policy->allowed_imports[policy->allowed_imports_count++] = import_name;
                 } else {
                     /* Try as atom */
                     char atom_buf[256];
                     if (enif_get_atom(env, head, atom_buf, sizeof(atom_buf), ERL_NIF_LATIN1)) {
                         char *import_name = enif_alloc(strlen(atom_buf) + 1);
-                        if (import_name != NULL) {
-                            strcpy(import_name, atom_buf);
-                            policy->allowed_imports[policy->allowed_imports_count++] = import_name;
+                        if (import_name == NULL) {
+                            /* Cleanup previously allocated imports on failure */
+                            for (size_t i = 0; i < policy->allowed_imports_count; i++) {
+                                enif_free(policy->allowed_imports[i]);
+                            }
+                            enif_free(policy->allowed_imports);
+                            policy->allowed_imports = NULL;
+                            policy->allowed_imports_count = 0;
+                            return -1;
                         }
+                        strcpy(import_name, atom_buf);
+                        policy->allowed_imports[policy->allowed_imports_count++] = import_name;
                     }
                 }
             }
@@ -565,7 +583,9 @@ int parse_sandbox_options(ErlNifEnv *env, ERL_NIF_TERM opts, sandbox_policy_t *p
  */
 void sandbox_policy_set_enabled(sandbox_policy_t *policy, bool enabled) {
     if (policy != NULL) {
+        pthread_mutex_lock(&policy->mutex);
         policy->enabled = enabled;
+        pthread_mutex_unlock(&policy->mutex);
     }
 }
 
@@ -592,6 +612,13 @@ int sandbox_policy_update(ErlNifEnv *env, sandbox_policy_t *policy, ERL_NIF_TERM
     temp.log_events = policy->log_events;
 
     if (parse_sandbox_options(env, opts, &temp) < 0) {
+        /* Clean up any imports allocated in temp before returning */
+        if (temp.allowed_imports != NULL) {
+            for (size_t i = 0; i < temp.allowed_imports_count; i++) {
+                enif_free(temp.allowed_imports[i]);
+            }
+            enif_free(temp.allowed_imports);
+        }
         pthread_mutex_unlock(&policy->mutex);
         return -1;
     }
@@ -601,18 +628,16 @@ int sandbox_policy_update(ErlNifEnv *env, sandbox_policy_t *policy, ERL_NIF_TERM
     policy->enabled = temp.enabled;
     policy->log_events = temp.log_events;
 
-    /* Update import whitelist */
-    if (temp.allowed_imports != NULL) {
-        /* Free old whitelist */
-        if (policy->allowed_imports != NULL) {
-            for (size_t i = 0; i < policy->allowed_imports_count; i++) {
-                enif_free(policy->allowed_imports[i]);
-            }
-            enif_free(policy->allowed_imports);
+    /* Update import whitelist - always sync from temp so omitting
+     * allow_imports in an update clears any existing whitelist */
+    if (policy->allowed_imports != NULL) {
+        for (size_t i = 0; i < policy->allowed_imports_count; i++) {
+            enif_free(policy->allowed_imports[i]);
         }
-        policy->allowed_imports = temp.allowed_imports;
-        policy->allowed_imports_count = temp.allowed_imports_count;
+        enif_free(policy->allowed_imports);
     }
+    policy->allowed_imports = temp.allowed_imports;
+    policy->allowed_imports_count = temp.allowed_imports_count;
 
     pthread_mutex_unlock(&policy->mutex);
     return 0;

c_src/py_sandbox.h

@@ -119,12 +119,14 @@ struct sandbox_policy_t {
     /** @brief Number of entries in allowed_imports */
     size_t allowed_imports_count;
 
-    /** @brief Whether to send audit events to Erlang handler */
+    /**
+     * @brief Whether to log audit events (reserved for future use)
+     *
+     * When true, blocked events will be logged. Event forwarding to Erlang
+     * is planned for a future version.
+     */
     bool log_events;
 
-    /** @brief PID of Erlang process to receive audit events */
-    ErlNifPid audit_handler;
-
     /** @brief Mutex for protecting policy updates */
     pthread_mutex_t mutex;
 };

docs/sandbox.md

@@ -37,7 +37,7 @@ py_nif:worker_destroy(Worker).
 ### Strict Preset
 
 The `strict` preset blocks:
-- `subprocess`: `subprocess.Popen`, `os.system`, `os.exec*`, `os.spawn*`, `os.popen`
+- `subprocess`: `subprocess.Popen`, `os.system`, `os.exec*`, `os.spawn*`, `os.fork`, `os.posix_spawn`, `os.popen`
 - `network`: `socket.*` operations
 - `ctypes`: ctypes module (memory access)
 - `file_write`: `open()` with write/append modes (`w`, `a`, `x`, `+`)
@@ -67,7 +67,7 @@ For fine-grained control, specify exactly which operations to block:
 |------|--------|
 | `file_write` | `open()` with write/append modes |
 | `file_read` | All file read operations |
-| `subprocess` | `subprocess.*`, `os.exec*`, `os.spawn*`, `os.popen` |
+| `subprocess` | `subprocess.*`, `os.exec*`, `os.spawn*`, `os.fork`, `os.posix_spawn`, `os.popen` |
 | `network` | `socket.*` operations |
 | `ctypes` | ctypes module (memory access) |
 | `import` | Non-whitelisted imports |

src/py.erl

@@ -841,7 +841,7 @@ ctx_exec(#py_ctx{ref = CtxRef}, Code) ->
 %% '''
 %%
 %% With strict preset, the following are blocked:
-%% - subprocess: subprocess.*, os.system, os.exec*, os.spawn*, os.popen
+%% - subprocess: subprocess.*, os.system, os.exec*, os.spawn*, os.fork, os.posix_spawn, os.popen
 %% - network: socket.*
 %% - ctypes: ctypes module (memory access)
 %% - file_write: open() with write/append modes

test/py_sandbox_SUITE.erl

@@ -28,9 +28,9 @@
     test_call_sandboxed_basic/1,
     test_call_sandboxed_strict/1,
     test_call_sandboxed_with_kwargs/1,
-    %% Escape attempt tests
-    test_subprocess_escape_attempts/1,
-    test_network_escape_attempts/1
+    %% Additional tests
+    test_file_write_escape_attempts/1,
+    test_file_read_allowed/1
 ]).
 
 all() ->
@@ -49,8 +49,8 @@ all() ->
         test_call_sandboxed_basic,
         test_call_sandboxed_strict,
         test_call_sandboxed_with_kwargs,
-        test_subprocess_escape_attempts,
-        test_network_escape_attempts
+        test_file_write_escape_attempts,
+        test_file_read_allowed
     ].
 
 init_per_suite(Config) ->
@@ -229,17 +229,23 @@ test_sandbox_enable_disable(_Config) ->
 
 %% Test sandbox_set_policy to change policy
 test_sandbox_set_policy(_Config) ->
-    {ok, W} = py_nif:worker_new(),
-    %% Initially no sandbox - file write allowed
-    ok = py_nif:worker_exec(W, <<"f = open('/tmp/policy_test_before.txt', 'w')\nf.close()">>),
-    %% Set policy to block file_write
-    ok = py_nif:sandbox_set_policy(W, #{enabled => true, block => [file_write]}),
-    %% Now file write should be blocked
-    Result = py_nif:worker_exec(W, <<"g = open('/tmp/policy_test_after.txt', 'w')">>),
-    py_nif:worker_destroy(W),
-    case Result of
-        {error, {'PermissionError', _}} -> ok;
-        Other -> ct:fail({should_be_blocked_after_policy_change, Other})
+    try
+        {ok, W} = py_nif:worker_new(),
+        %% Initially no sandbox - file write allowed
+        ok = py_nif:worker_exec(W, <<"f = open('/tmp/policy_test_before.txt', 'w')\nf.close()">>),
+        %% Set policy to block file_write
+        ok = py_nif:sandbox_set_policy(W, #{enabled => true, block => [file_write]}),
+        %% Now file write should be blocked
+        Result = py_nif:worker_exec(W, <<"g = open('/tmp/policy_test_after.txt', 'w')">>),
+        py_nif:worker_destroy(W),
+        case Result of
+            {error, {'PermissionError', _}} -> ok;
+            Other -> ct:fail({should_be_blocked_after_policy_change, Other})
+        end
+    after
+        %% Clean up test files
+        _ = file:delete("/tmp/policy_test_before.txt"),
+        _ = file:delete("/tmp/policy_test_after.txt")
     end.
 
 %% Test sandbox_get_policy
@@ -284,11 +290,11 @@ test_call_sandboxed_with_kwargs(_Config) ->
     ok.
 
 %%% ============================================================================
-%%% Test Cases - Escape Attempts
+%%% Test Cases - Additional File Operation Tests
 %%% ============================================================================
 
-%% Test various file write escape attempts
-test_subprocess_escape_attempts(_Config) ->
+%% Test that various file write modes are blocked
+test_file_write_escape_attempts(_Config) ->
     %% Test each write attempt with a fresh worker
     %% Using file operations since they are reliably audited
     Attempts = [
@@ -309,8 +315,8 @@ test_subprocess_escape_attempts(_Config) ->
     end, Attempts),
     ok.
 
-%% Test that file reads are allowed
-test_network_escape_attempts(_Config) ->
+%% Test that file reads are allowed with strict preset
+test_file_read_allowed(_Config) ->
     {ok, W} = py_nif:worker_new(#{sandbox => #{preset => strict}}),
     %% File read should be allowed (strict only blocks write)
     Result = py_nif:worker_exec(W, <<"data = open('/etc/hosts', 'r').read()[:10]">>),