Conversation
chu11
force-pushed
the
libsubprocess_misc_cleanup_4
branch
from
August 4, 2026 22:13
32d4cbc to
9dedb55
Compare
chu11
force-pushed
the
libsubprocess_misc_cleanup_4
branch
from
September 9, 2026 17:19
9dedb55 to
b340ed1
Compare
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>
Problem: Several internal function names in bulk-exec were named when bulk-exec was in the job-exec module. Now that the code is located in libsubprocess, these function names give the appearance the functions are not internal to bulk-exec. To avoid funciton name similarities, prefix several internal functions with "bulk_exec" so it is clear these are bulk-exec specific functions.
Problem: Some code does not conform to modern project coding style per RFC7. Fix some indentation and break long parameter lists into one per line.
chu11
force-pushed
the
libsubprocess_misc_cleanup_4
branch
from
September 9, 2026 20:53
b340ed1 to
3b42820
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #7744 +/- ##
==========================================
- Coverage 83.83% 83.81% -0.02%
==========================================
Files 599 599
Lines 102706 102758 +52
==========================================
+ Hits 86101 86126 +25
- Misses 16605 16632 +27
🚀 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.