devs/cproof
1
2
Fork 1
Code Issues 43 Pull Requests 6 Actions Packages Projects 1 Releases Wiki Activity

WIP: fix: address GCC -fanalyzer warnings #107

Closed
jabber.developer2 wants to merge 1 commits from fix/analyzer-warnings into master
pull from: fix/analyzer-warnings
merge into: devs:master
Conversation 1 Commits 1 Files Changed 2 +14 -1
jabber.developer2 commented 2026-04-17 10:49:34 +00:00
Collaborator
Copy Link

Summary

Re-enable the static-analysis warnings that were reverted in PR #105
(a7ad9ac0a Revert build: enable -Wconversion/-Wsign-compare and add --enable-sanitizers), and ship the remaining -fanalyzer null-pointer
guards that were not absorbed into master through other commits.

Goal: get back to a state where the next round of upstream-style refactors
can't silently re-introduce narrowing / sign / unsigned-overflow bugs.

Scope

1. Null-pointer guards (already present in this PR, 0ad7327fe)

Found by GCC -fanalyzer (GCC 15.2.1). Not covered by upstream.

  • src/command/cmd_funcs.c: check strtok() return before use in
    cmd_process_input.
  • src/command/cmd_funcs.c: check fdopen() return before use in
    cmd_sendfile.
  • src/omemo/omemo.c: init fingerprint_len = 0 and check
    _omemo_fingerprint_decode() return in omemo_is_trusted_identity
    (CWE-457 / CWE-690).
  • src/omemo/omemo.c: early-NULL return in _omemo_fingerprint_decode()
    for NULL input.

2. Re-enable conversion / sign warnings (advisory)

configure.ac currently silences -Wsign-compare outright and never
enables -Wsign-conversion or -Wconversion. PR #105 (a7ad9ac0a)
reverted the previous attempt to turn them on because the upstream merge
brought in ~230 sign warnings.

Now that PR #105's narrowing-conversion pass (9feff00ea) has landed
upstream-aligned types in the touched files, re-enable the flags in
advisory mode (warnings, not errors):

  • Drop -Wno-sign-compare from the default AM_CFLAGS.
  • Add -Wconversion -Wsign-conversion -Wsign-compare to the default flag
    set, gated by -Wno-error=conversion -Wno-error=sign-conversion -Wno-error=sign-compare so existing builds stay green while the
    warning floor surfaces the remaining hits.
  • Keep -Werror only for the unused-class warnings (current dev-mode
    policy in AS_IF([test "x$PACKAGE_STATUS" = xdevelopment], …)).

This restores visibility of new offenders introduced by future merges
without breaking distro builds today.

3. --enable-sanitizers opt-in switch

Re-add the configure switch reverted in a7ad9ac0a. When set:

  • -fsanitize=address
  • -fsanitize=undefined
  • -fsanitize=unsigned-integer-overflow (specifically catches the
    unsigned-subtraction footguns the PR #105 review flagged in
    statusbar.c / window.c).

Off by default.

What's deliberately not in this PR

  • No -Werror for the new sign/conversion warnings yet. That comes
    after each subsystem-author has a chance to clean their files. Tracked
    per-file in issue #112 (the "complete signed→unsigned hardening"
    section).
  • No src/tools/numutil.h helpers. At present every existing
    unsigned subtraction in tree is protected by an explicit pre-check or
    a comparison-rewrite, so YAGNI. Revisit if 3+ awkward sites accumulate.
  • No -fanalyzer false-positive suppressions. Older revisions of this
    PR contained #pragma GCC diagnostic pairs around iq.c callback
    registration sites; those were either absorbed into master via #2094
    cleanups or proved unnecessary after subsequent refactors. Drop.

Acceptance criteria

  • 0ad7327fe (null-pointer guards) compiles clean and tests pass on
    all four ci-build.sh configs.
  • configure.ac changes:
    • -Wno-sign-compare removed from AM_CFLAGS.
    • -Wconversion -Wsign-conversion -Wsign-compare added.
    • -Wno-error=conversion -Wno-error=sign-conversion -Wno-error=sign-compare gating present.
  • --enable-sanitizers switch wires up -fsanitize=address,
    -fsanitize=undefined, -fsanitize=unsigned-integer-overflow.
  • ci-build.sh log shows the new warning count (so we know the
    baseline for follow-up cleanup); update issue #112's
    "signed/unsigned hardening" section with that number.

Out of scope (issue #112 follow-up)

  • File-by-file cleanup of the ~230 sign warnings the new flags will
    uncover.
  • Promoting -Wno-error=sign-* to -Werror=sign-* once the warnings
    are gone.
  • numutil.h if/when we accumulate enough awkward sites to justify
    the abstraction.
## Summary Re-enable the static-analysis warnings that were reverted in PR #105 (`a7ad9ac0a Revert build: enable -Wconversion/-Wsign-compare and add --enable-sanitizers`), and ship the remaining `-fanalyzer` null-pointer guards that were not absorbed into master through other commits. Goal: get back to a state where the next round of upstream-style refactors can't silently re-introduce narrowing / sign / unsigned-overflow bugs. ## Scope ### 1. Null-pointer guards (already present in this PR, `0ad7327fe`) Found by GCC `-fanalyzer` (GCC 15.2.1). Not covered by upstream. - `src/command/cmd_funcs.c`: check `strtok()` return before use in `cmd_process_input`. - `src/command/cmd_funcs.c`: check `fdopen()` return before use in `cmd_sendfile`. - `src/omemo/omemo.c`: init `fingerprint_len = 0` and check `_omemo_fingerprint_decode()` return in `omemo_is_trusted_identity` (CWE-457 / CWE-690). - `src/omemo/omemo.c`: early-NULL return in `_omemo_fingerprint_decode()` for NULL input. ### 2. Re-enable conversion / sign warnings (advisory) `configure.ac` currently silences `-Wsign-compare` outright and never enables `-Wsign-conversion` or `-Wconversion`. PR #105 (`a7ad9ac0a`) reverted the previous attempt to turn them on because the upstream merge brought in ~230 sign warnings. Now that PR #105's narrowing-conversion pass (`9feff00ea`) has landed upstream-aligned types in the touched files, re-enable the flags in advisory mode (warnings, not errors): - Drop `-Wno-sign-compare` from the default `AM_CFLAGS`. - Add `-Wconversion -Wsign-conversion -Wsign-compare` to the default flag set, **gated** by `-Wno-error=conversion -Wno-error=sign-conversion -Wno-error=sign-compare` so existing builds stay green while the warning floor surfaces the remaining hits. - Keep `-Werror` only for the `unused`-class warnings (current dev-mode policy in `AS_IF([test "x$PACKAGE_STATUS" = xdevelopment], …)`). This restores visibility of new offenders introduced by future merges without breaking distro builds today. ### 3. `--enable-sanitizers` opt-in switch Re-add the configure switch reverted in `a7ad9ac0a`. When set: - `-fsanitize=address` - `-fsanitize=undefined` - `-fsanitize=unsigned-integer-overflow` (specifically catches the unsigned-subtraction footguns the PR #105 review flagged in `statusbar.c` / `window.c`). Off by default. ## What's deliberately *not* in this PR - **No `-Werror` for the new sign/conversion warnings yet.** That comes after each subsystem-author has a chance to clean their files. Tracked per-file in issue #112 (the "complete signed→unsigned hardening" section). - **No `src/tools/numutil.h` helpers.** At present every existing unsigned subtraction in tree is protected by an explicit pre-check or a comparison-rewrite, so YAGNI. Revisit if 3+ awkward sites accumulate. - **No -fanalyzer false-positive suppressions.** Older revisions of this PR contained `#pragma GCC diagnostic` pairs around `iq.c` callback registration sites; those were either absorbed into master via #2094 cleanups or proved unnecessary after subsequent refactors. Drop. ## Acceptance criteria - [ ] `0ad7327fe` (null-pointer guards) compiles clean and tests pass on all four `ci-build.sh` configs. - [ ] `configure.ac` changes: - [ ] `-Wno-sign-compare` removed from `AM_CFLAGS`. - [ ] `-Wconversion -Wsign-conversion -Wsign-compare` added. - [ ] `-Wno-error=conversion -Wno-error=sign-conversion -Wno-error=sign-compare` gating present. - [ ] `--enable-sanitizers` switch wires up `-fsanitize=address`, `-fsanitize=undefined`, `-fsanitize=unsigned-integer-overflow`. - [ ] `ci-build.sh` log shows the new warning count (so we know the baseline for follow-up cleanup); update issue #112's "signed/unsigned hardening" section with that number. ## Out of scope (issue #112 follow-up) - File-by-file cleanup of the ~230 sign warnings the new flags will uncover. - Promoting `-Wno-error=sign-*` to `-Werror=sign-*` once the warnings are gone. - `numutil.h` if/when we accumulate enough awkward sites to justify the abstraction.
jabber.developer2 added 1 commit 2026-04-17 10:49:37 +00:00
fix: address GCC -fanalyzer warnings
Some checks failed
CI Code / Code Coverage (pull_request) Failing after 11m32s
Details
CI Code / Check spelling (pull_request) Failing after 11m46s
Details
CI Code / Check coding style (pull_request) Failing after 12m12s
Details
CI Code / Linux (ubuntu) (pull_request) Failing after 12m40s
Details
CI Code / Linux (debian) (pull_request) Failing after 12m53s
Details
CI Code / Linux (arch) (pull_request) Failing after 13m9s
Details
d3ab7eb0e3 
- profanity.c: initialize 'waittime' to 0 (CWE-457)
- log.c: remove redundant close(STDERR_FILENO) before dup2 (CWE-687)
- cmd_funcs.c: close file handle on OMEMO error path (CWE-401)
- account.c: pclose() stream before early returns (CWE-401)
- omemo.c: fix double call to omemo_format_fingerprint() leak (CWE-401)
- http_download.c: free previous err before overwriting (CWE-401)
- iq.c: suppress false-positive malloc-leak from callback ownership
jabber.developer2 force-pushed fix/analyzer-warnings from d3ab7eb0e3 to 43aeaf6e3b 2026-04-17 13:25:25 +00:00 Compare
jabber.developer2 force-pushed fix/analyzer-warnings from 43aeaf6e3b to 0ad7327fea 2026-05-16 08:53:41 +00:00 Compare
jabber.developer2 commented 2026-06-19 08:39:44 +00:00
Author
Collaborator
Copy Link

Superseded — closing.

Re-checked against current master. The actual diff on this branch (commit 0ad7327fe) is four null-pointer guards; the -Wsign-compare/-Wconversion re-enable described above was never committed here (it belongs to the build-flags track, #109/#100, and master currently sets -Wno-sign-compare).

Per-hunk status:

cmd_process_input strtok guard — already on master (identical).
cmd_sendfile fdopen guard — already on master, and more complete there (it also close(fd) to avoid an fd leak); this PR's version would regress that.
omemo_is_trusted_identity (fingerprint_len = 0 + if (!fingerprint_raw) return FALSE) — still valid, absorbed into #130.
_omemo_fingerprint_decode NULL-input guard — still valid, absorbed into #130.
The only unique, still-relevant content (the two OMEMO CWE-476/457 guards) has moved to #130, so closing this PR.

Superseded — closing. Re-checked against current master. The actual diff on this branch (commit 0ad7327fe) is four null-pointer guards; the -Wsign-compare/-Wconversion re-enable described above was never committed here (it belongs to the build-flags track, #109/#100, and master currently sets -Wno-sign-compare). Per-hunk status: cmd_process_input strtok guard — already on master (identical). cmd_sendfile fdopen guard — already on master, and more complete there (it also close(fd) to avoid an fd leak); this PR's version would regress that. omemo_is_trusted_identity (fingerprint_len = 0 + if (!fingerprint_raw) return FALSE) — still valid, absorbed into #130. _omemo_fingerprint_decode NULL-input guard — still valid, absorbed into #130. The only unique, still-relevant content (the two OMEMO CWE-476/457 guards) has moved to #130, so closing this PR.
jabber.developer2 deleted branch fix/analyzer-warnings 2026-06-19 08:41:36 +00:00
All checks were successful
CI Code / Check spelling (pull_request) Successful in 17s
Required
Details
CI Code / Check coding style (pull_request) Successful in 32s
Required
Details
CI Code / Code Coverage (pull_request) Successful in 2m44s
Required
Details
CI Code / Linux (debian) (pull_request) Successful in 4m43s
Required
Details
CI Code / Linux (ubuntu) (pull_request) Successful in 4m58s
Required
Details
CI Code / Linux (arch) (pull_request) Successful in 5m49s
Required
Details

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
Compat/Breaking
Breaking change that won't be backward compatible
Kind/Bug
Something is not working
Kind/Documentation
Documentation changes
Kind/Enhancement
Improve existing functionality
Kind/Feature
New functionality
Kind/Security
This is security issue
Kind/Testing
Issue or pull request related to testing
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Low
4
The priority is low
Priority
Medium
3
The priority is medium
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
admin jabber.developer jabber.developer2
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: devs/cproof#107
No description provided.
Delete Branch "fix/analyzer-warnings"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?