cmd_ln_parse() return contract is untruthful: documented <0-on-error but calls exit(-1)

[lenzo-ka]

Summary

The deprecated non-reentrant cmd_ln_parse() API is documented to return 0 on success and a negative value on error, but its implementation calls exit(-1) when cmd_ln_parse_r() fails and otherwise always returns 0. This makes error checks unreachable and lets a library routine terminate its host process. There are two safe remedies: document the existing exit-on-error behavior, or implement the documented return contract and update every caller in the same change. Changing only the wrapper would turn currently loud failures into silent continuation at unchecked call sites.

Diagnosis

The declaration and implementation disagree. cmd_ln_parse_r() already has a normal NULL failure result; the legacy wrapper replaces that result with process termination. Consequently, a caller cannot observe the documented negative return.

The parser also has contract gaps adjacent to this choice:

• In non-strict mode, an unknown option is skipped, including its following token; duplicate options replace the earlier value. These behaviors are not described by the header's short strictness note.
• Integer conversion uses sscanf(..., "%ld", ...) without checking for trailing characters, and floating conversion uses atof_c(), so inputs such as 12junk are accepted as numeric prefixes and a nonnumeric float becomes 0.0.
• Defaults are inserted before required arguments are checked. A missing REQARG_STRING with the conventional empty-string default is therefore present by the time the check runs.
• The required-argument loop initializes a counter to zero but never increments it when it logs a missing argument, so even a genuine miss would not take the subsequent error branch.

A mechanical audit of a downstream integration (pstrain) found 37 cmd_ln_parse() call expressions outside the function definition. Exactly one checks the return; 36 discard it. That count includes integration-specific library wrappers and is evidence for migration risk, not a claim that this tree has the same wrapper set. The same mechanical audit should be rerun on the target revision here before choosing option B.

Exact code loci

Verified against master at commit 01461b25 (git log --oneline -1):

• include/sphinxbase/cmd_ln.h:426-433: the cmd_ln_parse() declaration; line 430 documents "@return 0 if successful, <0 if error."
• src/libs/libsphinxbase/util/cmd_ln.c:364-429: permissive integer and floating conversion.
• src/libs/libsphinxbase/util/cmd_ln.c:599-648: unknown-option and duplicate-option handling.
• src/libs/libsphinxbase/util/cmd_ln.c:651-679: defaults precede the required check, and the failure counter is never incremented.
• src/libs/libsphinxbase/util/cmd_ln.c:745-759: the wrapper calls exit(-1) (line 753) on NULL and otherwise returns only 0 (line 759).

Minimal reproduction

This small program demonstrates the public contract mismatch without depending on a SphinxTrain executable:

#include <sphinxbase/cmd_ln.h>
#include <stdio.h>

static const arg_t defn[] = {
    { "-known", ARG_STRING, NULL, "known option" },
    { NULL, 0, NULL, NULL }
};

int main(void)
{
    char *argv[] = { "probe", "-unknown", "value" };
    int rv = cmd_ln_parse(defn, 3, argv, 1);
    printf("returned %d\n", rv);
    return 0;
}

The header says this should print a negative return. Instead, the process prints the parse diagnostic and terminates from inside cmd_ln_parse() (commonly observed by a shell as status 255 because exit(-1) is truncated to eight bits).

For the required-argument bug, define { "-required", REQARG_STRING, "", ... }, parse an otherwise valid argument list that omits it, and inspect the returned configuration: the empty default has already been inserted, so parsing succeeds.

Proposed fix

Option A: documentation fix

Preserve compatibility and make the header explicit: cmd_ln_parse() terminates the process on parse failure and returns 0 only on success. Remove or rewrite any in-tree checks for a negative return, because they are dead under that contract. Document the strict/non-strict behavior table and the retained numeric-conversion behavior.

Independently, check required arguments before inserting defaults and increment the missing-required counter (or return immediately). Add focused tests for omitted required arguments.

This is the smallest behavioral change and preserves the current loud-failure property, but it also preserves the undesirable ability of a deprecated library API to terminate an embedding process.

Option B: atomic contract migration

Replace exit(-1) with a negative return, then update every caller in the same change:

• Standalone programs must convert parse failure to a nonzero process exit.
• Library entry points must propagate a failure to their caller and must not continue using the global configuration.
• Every formerly discarded call must receive a test or an explicit audit ruling.

Do not land the wrapper change alone: all unchecked callers would continue after a parse failure. Do not land caller checks first and describe the migration as complete: those checks remain unreachable until the wrapper changes. The wrapper and complete caller migration are one atomic correctness unit.

Option B should also fix required-argument ordering and its missing counter. Numeric parsing can either become full-token strict parsing or remain permissive with that behavior documented and tested.

mllr_transform is one concrete caller whose init-failure path is unreachable today for exactly this reason; it is filed as a companion issue.

Reference audit

A downstream integration (pstrain) briefly proved that returning a negative value made a malformed mllr_transform command reach its initialization diagnostic, then deliberately did not land that isolated change after the caller audit exposed the silent-continuation risk. The audit result — 37 external calls, one checked, 36 unchecked — is why both remedy options are presented here rather than a single wrapper patch.

---

Companion caller issue: #64 (mllr_transform init-failure path, unreachable today for this reason).