[#49] harden NULL handling and resource lifecycle across UI and SQLite #47

Manually merged
jabber.developer merged 7 commits from playground/fix/src_refactoring into master 2026-02-06 19:01:48 +00:00
Collaborator

Refactoring: security hardening, code deduplication, test coverage

Security

  • Fixed CWE-134 format string injection vulnerabilities across cons_show, log_error, win_println calls where user data was passed directly as format string
  • Added check-cwe134.sh static analyzer integrated into CI (code-style job) to prevent future format string vulnerabilities
  • Hardened NULL pointer handling and resource lifecycle management in UI components and SQLite database layer

Refactoring

  • Optimized hash table iterations using g_hash_table_iter instead of g_hash_table_foreach (#58)
  • Unified SQLite helper functions, reduced code duplication in database operations
  • Refactored window/subwin logic to eliminate redundant code paths
  • Refactored cmd_defs.c command definitions for improved metrics collection

Tests

  • Added functional tests for XEP-0012 (Last Activity) query/response handling
  • Added functional tests for XEP-0045 (MUC) room join and presence scenarios

CI

  • Improved CI stability with parallel Docker builds
  • Integrated CWE-134 security check into code-style workflow

Docs

  • Updated CONTRIBUTING.md with security checks section

Closes #49, #58, #85

**Refactoring: security hardening, code deduplication, test coverage** ### Security - Fixed CWE-134 format string injection vulnerabilities across `cons_show`, `log_error`, `win_println` calls where user data was passed directly as format string - Added check-cwe134.sh static analyzer integrated into CI (code-style job) to prevent future format string vulnerabilities - Hardened NULL pointer handling and resource lifecycle management in UI components and SQLite database layer ### Refactoring - Optimized hash table iterations using `g_hash_table_iter` instead of `g_hash_table_foreach` (#58) - Unified SQLite helper functions, reduced code duplication in database operations - Refactored window/subwin logic to eliminate redundant code paths - Refactored `cmd_defs.c` command definitions for improved metrics collection ### Tests - Added functional tests for XEP-0012 (Last Activity) query/response handling - Added functional tests for XEP-0045 (MUC) room join and presence scenarios ### CI - Improved CI stability with parallel Docker builds - Integrated CWE-134 security check into code-style workflow ### Docs - Updated CONTRIBUTING.md with security checks section Closes #49, #58, #85
jabber.developer2 added 1 commit 2025-11-09 19:14:30 +00:00
fix(ui,db): harden NULL handling and resource lifecycle across UI and SQLite
Some checks failed
CI Code / Linux (arch) (pull_request) Failing after 20s
CI Code / Check spelling (pull_request) Successful in 24s
CI Code / Check coding style (pull_request) Failing after 36s
CI Code / Linux (ubuntu) (pull_request) Successful in 15m27s
CI Code / Linux (debian) (pull_request) Successful in 17m51s
332cd243c6
ui/window: fix subwindow lifecycle (safe delwin on recreate), clamp widths, add fallback timestamp
when loading history to avoid NULL deref
ui/buffer: add GSList bounds checks; assert non-NULL timestamps when creating entries
ui/titlebar: guard newwin failures; make draw/resize/free no-ops when window is NULL
ui/statusbar: guard window creation/resize/close; clamp columns; fallback display name if JID
parsing fails
ui/inputwin: check newpad result; guard resize/getters/close on NULL; safe delwin
ui/chatwin: guard buffer_get_entry/time before ISO8601 formatting
ui/window_list: validate win_create_* results; don’t insert NULL windows; fix barejid leak in
wins_get_by_string
db/database: ensure cleanup on sqlite init/open failures; use sqlite3_close_v2 and warn if busy;
always return a ProfMessage from log_database_get_limits_info and set current UTC timestamp when
is_last with no row; initialize err_msg and free consistently; improve error messages
Prevents crashes from NULL dereferences (e.g., during MAM history) and closes small leaks; improves
robustness under OOM and allocation failures.
jabber.developer changed title from fix(ui,db): harden NULL handling and resource lifecycle across UI and SQLite to [#49] harden NULL handling and resource lifecycle across UI and SQLite 2025-11-10 06:59:09 +00:00
jabber.developer2 changed title from [#49] harden NULL handling and resource lifecycle across UI and SQLite to WIP: [#49] harden NULL handling and resource lifecycle across UI and SQLite 2025-11-10 08:11:33 +00:00
jabber.developer2 added 1 commit 2025-11-10 12:18:10 +00:00
feat(ui,db,cmd): reduce dublicate code in window subwin logic, unify SQLite helpers, NULL checks added
Some checks failed
CI Code / Check spelling (pull_request) Successful in 18s
CI Code / Linux (arch) (pull_request) Failing after 22s
CI Code / Check coding style (pull_request) Failing after 31s
CI Code / Linux (debian) (pull_request) Successful in 15m14s
CI Code / Linux (ubuntu) (pull_request) Successful in 15m36s
5f01f8f546
window.c: add _get_subwin_cols, _apply_split_resize, _check_subwin_width; refactor show/hide/resize/refresh/update
database.c: add _db_prepare_ctx, _db_teardown; safer init/queries with fallback timestamp
cmd_defs.c: guard fopen in docgen; inputwin.c: replace assert with runtime check
jabber.developer2 added 1 commit 2025-11-10 12:45:43 +00:00
fix: Refactor src/command/cmd_defs.c for metrics fix
All checks were successful
CI Code / Check spelling (pull_request) Successful in 19s
CI Code / Check coding style (pull_request) Successful in 36s
CI Code / Linux (ubuntu) (pull_request) Successful in 15m35s
CI Code / Linux (debian) (pull_request) Successful in 17m52s
CI Code / Linux (arch) (pull_request) Successful in 9m23s
33952c1331
jabber.developer2 added 1 commit 2025-11-10 15:51:30 +00:00
refactor: optimize hash table iterations and fix security issues #58
All checks were successful
CI Code / Check spelling (pull_request) Successful in 21s
CI Code / Check coding style (pull_request) Successful in 36s
CI Code / Linux (arch) (pull_request) Successful in 10m15s
CI Code / Linux (ubuntu) (pull_request) Successful in 13m57s
CI Code / Linux (debian) (pull_request) Successful in 16m17s
b7a38f1e34
- refactor(core): replace g_hash_table_get_keys with g_hash_table_iter_init
  * Eliminates temporary GList allocations
  * Improves iteration performance
  * Affected: connection.c, cmd_defs.c, cmd_funcs.c, omemo.c, gpg.c,
    disco.c, form.c, autocompleters.c, capabilities.c, callbacks.c

- fix(xmpp): correct queued_messages loop in connection.c:1031
  * Remove incorrect NULL check that prevented message storage
  * calloc zeros array, causing loop to skip immediately
  * Fixes dropped messages during reconnection with SM enabled

- fix(ui): prevent format string vulnerabilities in cons_show calls
  * Replace cons_show(variable) with cons_show("%s", variable)
  * Protects against format string attacks if variables contain %
  * Updated instances across cmd_funcs.c, connection.c, ox.c,
    console.c, core.c
jabber.developer reviewed 2025-11-19 16:31:24 +00:00
src/database.c Outdated
@@ -68,0 +77,4 @@
}
g_chatlog_database = NULL;
}
sqlite3_shutdown();

Earlier it was conditional. Could it cause an issue to have unconditional sqlite3_shutdown?

Also the condition takes almost full function. Maybe it would make sense to use early exit statement if (!g_chatlog_database) return;?

Earlier it was conditional. Could it cause an issue to have unconditional sqlite3_shutdown? Also the condition takes almost full function. Maybe it would make sense to use early exit statement `if (!g_chatlog_database) return;`?
Author
Collaborator

Unconditional sqlite3_shutdown() is safe — it's a no-op if SQLite is already shut down.

https://www.sqlite.org/c3ref/initialize.html

Unconditional sqlite3_shutdown() is safe — it's a no-op if SQLite is already shut down. https://www.sqlite.org/c3ref/initialize.html
jabber.developer marked this conversation as resolved
src/database.c Outdated
@@ -286,0 +315,4 @@
if (!myjid || !myjid->str) {
// If caller requested the last message and we have no context, fall back to now
if (is_last) {
msg->timestamp = g_date_time_new_now_utc();

Wouldn't this cause a faulty logic on iq_mam_request_older?

src/xmpp/iq.c Lines 2654 to 2660 in 74cfd32c1e
// If first message found
if (first_msg->timestamp) {
firstid = first_msg->stanzaid;
enddate = g_date_time_format(first_msg->timestamp, mam_timestamp_format_string);
} else {
return;
}

Wouldn't this cause a faulty logic on `iq_mam_request_older`? https://git.jabber.space/devs/cproof/src/commit/74cfd32c1ec981f3ad186fbee998c10a473df45b/src/xmpp/iq.c#L2654-L2660
jabber.developer marked this conversation as resolved
src/database.c Outdated
@@ -302,1 +338,3 @@
return NULL;
if (!_db_prepare_ctx(query, &stmt, "log_database_get_limits_info()")) {
if (is_last) {
msg->timestamp = g_date_time_new_now_utc();

Analogous to the previous comment.

Analogous to the previous comment.
jabber.developer marked this conversation as resolved
@@ -164,2 +167,4 @@
inp_win = newpad(1, INP_WIN_MAX);
if (!inp_win) {
// Failed to allocate input pad; leave inp_win NULL and avoid further use

Maybe we should add error logging for gracefulness?

Maybe we should add error logging for gracefulness?

Not resolved; no response.

Not resolved; no response.
Author
Collaborator

Added

Added
jabber.developer marked this conversation as resolved
@@ -111,16 +111,23 @@ status_bar_init(void)
int row = screen_statusbar_row();
int cols = getmaxx(stdscr);
if (cols <= 0) {

Is it realistic condition? It doesn't appear a normal state, so maybe we should log_warn it?

Is it realistic condition? It doesn't appear a normal state, so maybe we should log_warn it?

Not resolved, as no response given/action taken.

Not resolved, as no response given/action taken.
Author
Collaborator

Added

Added
jabber.developer marked this conversation as resolved
@@ -676,3 +692,2 @@
auto_jid Jid* jid = jid_create(tab->identifier);
auto_gchar gchar* mucwin_title = mucwin_generate_title(jid->barejid, PREF_STATUSBAR_ROOM_TITLE);
fullname = g_strconcat(mucwin_title, "/", jid->resourcepart, NULL);
if (jid) {

Nice catch!

Nice catch!
jabber.developer marked this conversation as resolved

src/ui/console.c:941

# src/ui/console.c:941
jabber.developer2 force-pushed playground/fix/src_refactoring from b7a38f1e34 to 58dd89be40 2026-02-04 13:21:33 +00:00 Compare
jabber.developer2 added 1 commit 2026-02-04 21:02:02 +00:00
tests: add autoping, helpers and XEP functional tests
All checks were successful
CI Code / Check spelling (pull_request) Successful in 20s
CI Code / Check coding style (pull_request) Successful in 35s
CI Code / Code Coverage (pull_request) Successful in 4m54s
CI Code / Linux (ubuntu) (pull_request) Successful in 6m23s
CI Code / Linux (debian) (pull_request) Successful in 9m12s
CI Code / Linux (arch) (pull_request) Successful in 11m12s
6f136bde88
- Add 15 unit tests for autoping logic (XEP-0199)
- Add 10 helper tests for refactoring validation
- Add Last Activity functional tests (XEP-0012)
- Add MUC affiliation/kick tests (XEP-0045)
jabber.developer2 added 1 commit 2026-02-05 14:23:07 +00:00
fix(security): prevent CWE-134 format string injection
All checks were successful
CI Code / Check spelling (pull_request) Successful in 20s
CI Code / Check coding style (pull_request) Successful in 36s
CI Code / Code Coverage (pull_request) Successful in 5m2s
CI Code / Linux (ubuntu) (pull_request) Successful in 6m30s
CI Code / Linux (debian) (pull_request) Successful in 9m5s
CI Code / Linux (arch) (pull_request) Successful in 10m53s
58c23c4089
- add "%s" to unsafe cons_show/log_*/win_println calls
- add check-cwe134.sh static analysis script
- integrate security check into CI pipeline

Closes #85
jabber.developer2 force-pushed playground/fix/src_refactoring from 58c23c4089 to 7d7615f10d 2026-02-05 15:14:09 +00:00 Compare
jabber.developer2 force-pushed playground/fix/src_refactoring from 7d7615f10d to 22335ca83e 2026-02-05 15:55:30 +00:00 Compare
jabber.developer2 force-pushed playground/fix/src_refactoring from 22335ca83e to 34501d913c 2026-02-05 15:58:15 +00:00 Compare
jabber.developer reviewed 2026-02-05 18:03:18 +00:00
jabber.developer left a comment
Owner

Please fix/comment earlier suggestions and address these 2 moments.

Please fix/comment earlier suggestions and address these 2 moments.
@@ -149,2 +149,3 @@
*/
assert(MB_CUR_MAX <= PROF_MB_CUR_MAX);
if (MB_CUR_MAX > PROF_MB_CUR_MAX) {
cons_show_error("Your locale's MB_CUR_MAX (%zu) exceeds PROF_MB_CUR_MAX (%d); input window disabled.", (size_t)MB_CUR_MAX, PROF_MB_CUR_MAX);

Technically correct, but what user is supposed to do with this data? Can we provide some actionable advise or explanation?

Technically correct, but what user is supposed to do with this data? Can we provide some actionable advise or explanation?
Author
Collaborator

Comment added

Comment added
jabber.developer marked this conversation as resolved
src/ui/window.c Outdated
@@ -78,0 +79,4 @@
static int
_check_subwin_width(int cols, int width)
{
if (cols > 1) {

Why not like this?

return cols <= 1 ? 1 : CLAMP(width, 1, cols - 1);
Why not like this? ```c return cols <= 1 ? 1 : CLAMP(width, 1, cols - 1); ```
Author
Collaborator

Corrected

Corrected
jabber.developer marked this conversation as resolved
jabber.developer changed title from WIP: [#49] harden NULL handling and resource lifecycle across UI and SQLite to [#49] harden NULL handling and resource lifecycle across UI and SQLite 2026-02-05 18:04:01 +00:00
jabber.developer requested review from jabber.developer 2026-02-05 18:07:42 +00:00
jabber.developer2 was assigned by jabber.developer 2026-02-05 18:07:43 +00:00
jabber.developer2 added 1 commit 2026-02-06 10:29:45 +00:00
fix(ui): address PR review comments
Some checks failed
CI Code / Check spelling (pull_request) Successful in 19s
CI Code / Check coding style (pull_request) Successful in 32s
CI Code / Code Coverage (pull_request) Failing after 36s
CI Code / Linux (arch) (pull_request) Failing after 55s
CI Code / Linux (debian) (pull_request) Failing after 1m11s
CI Code / Linux (ubuntu) (pull_request) Failing after 1m33s
7d392e38ca
- inputwin.c: improve MB_CUR_MAX error message with actionable advice
- inputwin.c: add log_error when input window pad allocation fails
- statusbar.c: add log_warning for invalid cols values
- window.c: simplify _check_subwin_width using CLAMP macro
Author
Collaborator

Suggested squash commit message:

fix(ui,db): harden NULL handling, fix CWE-134, optimize iterations

  • fix(security): CWE-134 format string injection + CI check
  • fix(ui): subwindow lifecycle, newwin/newpad guards, fallback timestamps
  • fix(db): sqlite cleanup on failures, sqlite3_close_v2
  • fix(xmpp): queued_messages loop, barejid leak
  • perf(core): g_hash_table_iter_init instead of g_hash_table_get_keys
  • refactor(ui): CLAMP macro in _check_subwin_width
  • test: XEP-0012 and XEP-0045 functional tests

Closes #58, #85

Suggested squash commit message: fix(ui,db): harden NULL handling, fix CWE-134, optimize iterations - fix(security): CWE-134 format string injection + CI check - fix(ui): subwindow lifecycle, newwin/newpad guards, fallback timestamps - fix(db): sqlite cleanup on failures, sqlite3_close_v2 - fix(xmpp): queued_messages loop, barejid leak - perf(core): g_hash_table_iter_init instead of g_hash_table_get_keys - refactor(ui): CLAMP macro in _check_subwin_width - test: XEP-0012 and XEP-0045 functional tests Closes #58, #85
jabber.developer2 force-pushed playground/fix/src_refactoring from 7d392e38ca to cbeaa13ed0 2026-02-06 10:42:09 +00:00 Compare
jabber.developer2 force-pushed playground/fix/src_refactoring from cbeaa13ed0 to 56dcb54ffc 2026-02-06 11:06:02 +00:00 Compare
jabber.developer approved these changes 2026-02-06 17:31:47 +00:00
jabber.developer left a comment
Owner

LGTM

LGTM
jabber.developer manually merged commit 467222d0ca into master 2026-02-06 19:01:48 +00:00
Sign in to join this conversation.
No description provided.