fix(RDKB-65467): handle ECHILD in exec_shell_cmd to suppress false DHCPv6 error log#129
Open
aadhithan01 wants to merge 9 commits into
Open
fix(RDKB-65467): handle ECHILD in exec_shell_cmd to suppress false DHCPv6 error log#129aadhithan01 wants to merge 9 commits into
aadhithan01 wants to merge 9 commits into
Conversation
…ror log system() returns -1 with errno=ECHILD when sigchld_handler reaps the child process before system()'s own waitpid() can collect it. This is a race condition where the command (e.g. 'ip -6 addr replace') runs successfully but the error path is triggered incorrectly. Root cause: sigchld_handler uses waitpid(-1, WNOHANG) which reaps ANY child, including the transient /bin/sh spawned by system(). When that happens, system() finds ECHILD and returns -1, causing a false ERROR log in configureNetworkInterface(). Fix: Check errno == ECHILD before treating system() return -1 as a failure. ECHILD here means the child already ran and was reaped - not that it failed to run. Log a WARNING instead and return success. The sigchld_handler / processKilled() flow is unaffected since the transient shell child pid is never registered as a DHCP client pid.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates DHCPMGR’s IPv6 lease handler to suppress a known false-negative error path where system() can return -1 with errno=ECHILD due to a SIGCHLD handler reaping the /bin/sh child before system() collects it, preventing misleading “Failed to configure IPv6 address” logs during boot.
Changes:
- Treat
system()returning-1witherrno==ECHILDas a non-fatal condition inexec_shell_cmd(). - Add a warning log for the ECHILD case and return success to avoid propagating a false failure.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add missing #include <errno.h> for errno and ECHILD usage - Reword log message to clarify exit status is unavailable (not confirmed success) and include the command string for easier debugging
- Change exec_shell_cmd parameter to const char * for const-correctness (function never modifies the command string) - Add #include <string.h> for strerror() - Improve real failure error log to include command, errno value and strerror description for easier field diagnosis
…issue Capture errno in saved_errno right after system() returns -1, before any other function call (including strerror()) can modify it. In C, argument evaluation order is unspecified and strerror() may alter errno, causing the numeric value and message to become inconsistent.
… SIGCHLD race system() is vulnerable to a race where sigchld_handler() (using waitpid(-1)) reaps the child before system()'s own waitpid() can collect it, causing system() to return -1/ECHILD even when the command succeeded. Silently treating ECHILD as success is incorrect because the exit status is genuinely unknown. Replace system() with an explicit fork/exec/waitpid pattern: - fork() a child and exec /bin/sh -c <command> - Parent uses waitpid(specific_pid) with EINTR retry loop - ECHILD case is now treated as a genuine error (exit status unknown) rather than silently swallowed - Add #include <signal.h> and <unistd.h> for signal/fork/execl/_exit This eliminates the false error log while correctly handling all failure modes without masking real command failures.
…rding - On ECHILD from waitpid(), return 0 with WARNING instead of ERROR+failure. The shell child is never a registered DHCP client pid so processKilled() is a no-op — this is safe and suppresses the false error log (core intent). - Restore non-ECHILD error path with errno+strerror for real failures. - Reword parent comment: says 'significantly reduces' not 'avoids' the race since ECHILD is still possible (just less likely with waitpid(pid)).
…status capture The definitive fix: mirror the sigprocmask pattern already used in start_exe2() in the same codebase. Block SIGCHLD before fork() and restore after waitpid() completes: - sigchld_handler cannot reap our child during the critical window - waitpid(pid) reliably captures the exit status — ECHILD is impossible - On unmask, any deferred SIGCHLD for other children fires normally so DHCP client selfheal notifications are not delayed or lost - Remove signal(SIGCHLD, SIG_DFL) in child: execl() resets signal dispositions automatically and signal() is not async-signal-safe between fork() and exec() in a multi-threaded process This eliminates the false error log without masking any real failures.
…ror log Revert to system()-based approach with explicit ECHILD handling. The fork/exec/waitpid+sigprocmask approach was overly complex: sigprocmask only blocks signals in the calling thread so ECHILD was still possible in a multithreaded process, requiring ECHILD handling regardless. The minimal correct fix: - Keep system() as the execution mechanism - Capture errno immediately after system() returns -1 - On ECHILD: log WARNING and return 0 (success) sigchld_handler's processKilled() is a no-op for transient shell children (never registered as DHCP client pids) so this is safe - On other errors: log ERROR with errno + strerror + command string - Change parameter to const char * (no mutation) - Add #include <errno.h> and <string.h>
- ECHILD comment: clarify exit status is unknown (not that command ran successfully), add 'exit status unavailable' to the warning log - Non-ECHILD error: change 'failed to fork shell' to 'fork/wait failed' since system() returns -1 for both fork and waitpid failures
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
source/DHCPMgrUtils/dhcpmgr_v6_lease_handler.c:59
- The Doxygen block immediately above exec_shell_cmd documents
processv6LesSysevents(brief/params/return) rather than exec_shell_cmd, which makes the API documentation misleading now that exec_shell_cmd is being updated. Please update this block to describe exec_shell_cmd (or move it to the correct function).
/**
* @brief processv6LesSysevents This function will set the sysevent values for IA_PD and IA_NA
*
* @param IPv6Events , size_t , const char*
*
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
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.
Problem
On XB8/XB9/XB10 bootup, the following false error is logged even though IPv6 address configuration succeeds:
Root Cause
A race condition between
system()andsigchld_handler():system()forks a child (/bin/sh -c "ip -6 addr replace ...")ip -6 addr replacesuccessfully and exits(0)sigchld_handler()fires, callswaitpid(-1, WNOHANG)— reaps any child including the shell childsystem()'s ownwaitpid(child_pid)gets errno=ECHILD — child already gonesystem()returns -1 → falsely logged as ERRORThe IPv6 address is actually configured on erouter0. This is a pure logging false positive.
Fix
In
exec_shell_cmd(), checkerrno == ECHILDbefore treatingsystem()returning-1as a failure:sigchld_handler'sprocessKilled()only acts on registered DHCP client pids — the transient/bin/shchild is never registered, so it is a no-op. This is safe.errno+strerror()+ command string and return -1.Additional improvements:
exec_shell_cmdparameter toconst char *#include <errno.h>and#include <string.h>errnointosaved_errnoimmediately aftersystem()to avoid argument evaluation order issuesWhy This Is Safe
sigchld_handler()→processKilled(pid)only acts on pids registered as DHCP client processes. The transient shell spawned bysystem()is never registered →processKilled()is a no-op for it. DHCP client crash selfheal notifications are unaffected.Test Procedure
References