Skip to content

Fix _Bool functions that silently discard -1 error returns - #424

Merged
thegushi merged 1 commit into
trusteddomainproject:developfrom
thegushi:fix/bool-return-collapse
Jul 22, 2026
Merged

Fix _Bool functions that silently discard -1 error returns#424
thegushi merged 1 commit into
trusteddomainproject:developfrom
thegushi:fix/bool-return-collapse

Conversation

@thegushi

Copy link
Copy Markdown
Collaborator

Summary

cppcheck static analysis turned up two instances of the same root cause: a function declared static _Bool that does return -1; on an error path. In C99/C11, a _Bool-returning function implicitly converts any nonzero int expression to 1 (TRUE) at the return statement, so these functions actually return TRUE, not -1, defeating callers that check for a tri-state (TRUE/FALSE/-1) contract.

  • dkim_sig_hdrlistok() (libopendkim/dkim.c): a DKIM_MALLOC failure is silently treated as "header list OK" instead of propagating DKIM_STAT_NORESOURCE. cppcheck confirms the caller-side dead code: dkim.c:2170: Condition 'hstatus==-1' is always false.
  • dkimf_parsehandler() (opendkim/opendkim.c): an invalid value for any On-* handling directive in the config file populates the error buffer, but the config-load call sites (!dkimf_parsehandler(...)) never see the failure, so config loading proceeds as if the directive were valid.

I grepped every _Bool-returning function in the tree for return -1; inside its body and confirmed these are the only two instances of the idiom.

Changes

  • Both functions changed from static _Bool to static int, keeping their existing TRUE(1)/FALSE(0)/-1 return values as literal ints.
  • Updated the dkimf_parsehandler() call sites in dkimf_config_load() from !dkimf_parsehandler(...) to dkimf_parsehandler(...) != TRUE, since both FALSE and -1 need to trip the error path.
  • dkim_sig_hdrlistok()'s one call site already used an int local for the result, so no caller change was needed there.

Test plan

  • autoreconf -fi && ./configure && gmake builds clean
  • gmake check: 174/174 tests pass

dkim_sig_hdrlistok() and dkimf_parsehandler() are declared to return
_Bool but use a tri-state contract (TRUE/FALSE/-1). In C99/C11, a
_Bool-returning function implicitly converts any nonzero return value
to 1 at the return statement, so `return -1;` actually returns TRUE to
the caller.

For dkim_sig_hdrlistok(), this means a DKIM_MALLOC failure is silently
treated as "header list OK" instead of propagating
DKIM_STAT_NORESOURCE.

For dkimf_parsehandler(), this means an invalid value for any On-*
handling directive in the config file populates the error buffer but
the invalid-value branch never causes config loading to fail, since
`!dkimf_parsehandler(...)` is false whether the function returns TRUE
or (as it actually does after the _Bool conversion) what should have
been -1.

Fixed by changing both functions to return int, and updating the
dkimf_parsehandler() call sites to check `!= TRUE` instead of `!`,
since FALSE and -1 both need to trip the error path.

cppcheck flags the caller-side dead code directly:
dkim.c:2170: Condition 'hstatus==-1' is always false
@thegushi
thegushi merged commit 7eb1939 into trusteddomainproject:develop Jul 22, 2026
2 checks passed
@thegushi
thegushi deleted the fix/bool-return-collapse branch July 22, 2026 02:16
thegushi added a commit that referenced this pull request Jul 22, 2026
thegushi added a commit that referenced this pull request Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant