libsubprocess: fix consistency issues and bugs - #7744
Open
chu11 wants to merge 28 commits into
Open
Conversation
Problem: local_channel_flush() calls subprocess_incref() then returns early if fbuf_read_watcher_get_buffer() fails, skipping the matching subprocess_decref(). Replace the early return with goto out so the decref runs on every path. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: server_killall() and exec_exit_notify() always returns 0 and all call sites ignore the return value. Change the return type to void. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: subprocess_destroy() returns -1 when flux_future_then() fails but never destroys the future created by flux_subprocess_kill(), leaking it. Destroy the future on the error path. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: subprocess_destroy_finish() looks up the "flux_t" aux key on the subprocess to log a kill failure, but nothing ever sets that key, so the handle is always NULL and flux_log_error() is called with a NULL handle, losing the log. Set the "flux_t" aux key in subprocess_destroy() before registering the continuation. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: two rexec.write error sites pass ": %s", strerror(errno) to log_err(), but log_err() already appends ": strerror(errno)", so the errno string is printed twice. Drop the redundant strerror argument. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: subprocess_check_completed() logs an "unexpected state" condition with log_err(), which appends ": strerror(errno)" -- a meaningless stale errno for a non-errno condition. Use log_msg() instead. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: subprocess_create() gated the atomic SOCK_CLOEXEC socketpair on "#if SOCK_CLOEXEC". On glibc SOCK_CLOEXEC is an enum constant (with a self-referential macro), so the preprocessor evaluates the identifier to 0 in #if, making the atomic branch dead and always using the non-atomic fallback. Use #ifdef, which correctly detects the macro definition. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: cmd_option_bufsize() and cmd_option_line_buffer() declare var uninitialized, then "goto cleanup" on asprintf failure where cleanup calls free(var). asprintf leaves *strp indeterminate on failure, so free(var) operates on a garbage pointer. Initialize var = NULL so the cleanup free is a safe no-op on the asprintf-failure path. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: flux_subprocess_write() compared fbuf_space() (int) against len (size_t) and passed len to fbuf_write()'s int parameter. Add explicit casts. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: argz_appendv() allocates s with vasprintf() but only frees it on the success path. When argz_add() fails, the function returns without freeing s, leaking it (argz_add copies the string and does not take ownership). Free s on the argz_add() failure path. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: the posix_spawn_file_actions_* and posix_spawnattr_setsig* functions return a positive errno on failure (0 on success), not -1/errno. spawn_setup_fds() and setup_signals() tested them with "< 0", so those checks were dead and failures went undetected; setup_signals() also returned setsigdefault()'s positive errno, which the caller tested with "< 0" and missed. Capture each return and convert to "!= 0" with errno = rc. The sigemptyset/sigaddset and fdwalk checks correctly use -1/errno and are left as < 0. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: flux_cmd_copy() left zlist_dup() and z_hash_dup() unchecked, so an allocation failure could produce a cmd with NULL channels/opts -- a silently partial copy. z_hash_dup() itself ignored zhash_new() and zhash_insert() failures and dereferenced a possibly-NULL hash via zhash_autofree(). Check both dups in flux_cmd_copy(), and check every allocation in z_hash_dup(), returning NULL on any failure. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: cmd_tojson() did not check the json_object() return for NULL, unlike the other *_tojson helpers in the file; on OOM it would pass NULL to json_object_set_new(). Add a NULL check that jumps to the existing err path. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: zhash_fromjson() and channels_fromjson() call zhash_autofree() / zlist_autofree() on the result of zhash_new() / zlist_new() without a NULL check. Both autofree functions assert(self) and dereference it, so an allocation failure asserts or crashes. Check the allocation and goto the existing fail path with errnum = ENOMEM, matching msgchans_fromjson(). Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: bulk_exec_create() did not check zlist_new() (x2) or idset_create() for NULL, so an allocation failure left exec->processes/commands/exit_batch NULL and later appends and idset operations would act on NULL. Check all three and goto the existing error path, which frees the partially-built struct via bulk_exec_destroy(). Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: server_disconnect_cb() passes flux_msg_route_first (p->waiter) directly to streq(), but flux_msg_route_first() can return NULL, which streq() (a bare strcmp) would dereference. Other call sites null-check the result first. Store the route and null-check it before streq(). Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: subprocess_childfds() ignores the return of idset_set() at three sites. The idset autogrows, so idset_set() can still fail on an allocation error; a silent failure would leave an fd unprotected and it would be closed in the child. Check each idset_set() and destroy the idset on failure. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: add_pending_signal() ignores the flux_subprocess_aux_set() return. On failure it still set p->signal_pending and incref'd the future, so fwd_pending_signal() would later run, aux_get NULL for "sp::signal_future", never fulfill the caller's future, and leak the incref'd reference. Destroy the future and return NULL on aux_set failure, before setting signal_pending or taking the extra reference. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: create_process_spawn() ignores the return value of several functions that can return errors. Check return values for cmd_env_expand(), cmd_argv_expand(), and spawn_setup_fds(). Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: env_entry_name() checks "len-1 < p - entry" with size_t len. If len is 0, len-1 underflows to SIZE_MAX, the truncation guard passes, and *dst = '\0' writes to a zero-size buffer. All current callers pass sizeof(buf)==1024, so this is latent. Reject len == 0 explicitly before the subtraction. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: The *_fromjson helpers in command.c report inconsistent errno values for the same "JSON is the wrong type" condition -- argz_fromjson, envz_fromjson, and msgchans_fromjson used EINVAL while zhash_fromjson and channels_fromjson use EPROTO. Standardize all five on EPROTO, matching how malformed input is reported elsewhere in the deserialization and RFC 42 server paths. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: argz_fromjson() and envz_fromjson() return EPROTO regardless of the error. Return ENOMEM when there is a memory allocation error.
Problem: bulk_exec_write() compares the int return of
flux_subprocess_write() against a size_t len ("< len"). On error the
-1 return converts to SIZE_MAX, so the comparison is false and the
write failure is silently swallowed. The len <= 0 guard is also a
dead comparison since len is unsigned.
Capture the return from flux_subprocess_write() in a signed int,
fail on < 0, and also fail with ENOSPC when fewer than len bytes
were buffered.
Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: server_write_cb() calls err_init() on its flux_error_t before server_auth_unpack(), but five other request handlers do not. If the auth callback path in server_auth_unpack() returns -1 without populating errp, those handlers read error.text uninitialized. Add err_init() before server_auth_unpack() in the exec, kill, list, wait, and attach handlers to match server_write_cb(). Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: flux_rexec_bg() performs no valid_flags mask check, unlike the other public exec entry points, so invalid flags pass through silently into the rexec request payload. Only LOCAL_UNBUF was rejected, and that check lives downstream in subprocess_rexec_bg(). Add a valid_flags check (NO_SETPGRP | FORK_EXEC | WAITABLE | SIGN) in flux_rexec_bg(), matching the sibling entry points; LOCAL_UNBUF and STDIO_FALLTHROUGH are now rejected there. Remove the now-unreachable LOCAL_UNBUF check in subprocess_rexec_bg(). Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: server_exec_cb() passes local_flags straight to flux_local_exec_ex() without screening LOCAL_UNBUF. LOCAL_UNBUF is a client-side output optimization with no meaning for a background subprocess launched by the server; it would fail late in flux_local_exec_ex() with a generic "error launching process" message. Reject LOCAL_UNBUF in background mode with a clear error, consistent with the adjacent stdio-fallthrough check. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: subprocess_rexec_bg() did not validate h or cmd, unlike its sibling entry points subprocess_rexec() and subprocess_rexec_attach(). A NULL cmd would be dereferenced in cmd_tojson(). Add an "if (!h || !cmd)" EINVAL guard. service_name is intentionally not checked, since NULL is valid here and defaults to "rexec". Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
Problem: local_release_child() handled read() returning < 0 (error) and sizeof(int) (exec error), then fell through assuming n == 0 (child exec'ed). A short read (0 < n < sizeof(int)) also fell through and was treated as a successful exec. Treat any n that is neither 0 nor sizeof(int) as a protocol error. Assisted-by: Claude:claude-opus-4-8 <noreply@anthropic.com>
chu11
force-pushed
the
libsubprocess_misc_cleanup_4
branch
from
August 4, 2026 22:13
32d4cbc to
9dedb55
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #7744 +/- ##
==========================================
- Coverage 83.94% 83.91% -0.03%
==========================================
Files 591 591
Lines 100922 100983 +61
==========================================
+ Hits 84716 84738 +22
- Misses 16206 16245 +39
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
part 4 of cleanups, from on top of #7743 (review earlier PRs first, there's a lot of commits).
This set of fixes are minor but takes a minor bit of thought :-) mostly they are consistency issues between functions (some check errors, some don't), or initialization inconsistencies, etc.