From 7eb966806869efb6650d4d7e77af9ad7ce66c4d2 Mon Sep 17 00:00:00 2001 From: "jabber.developer2" Date: Sun, 14 Jun 2026 17:06:37 +0300 Subject: [PATCH] build: Pikaur-safe hardening flags, opt-in sanitizers, -Wsign-compare Re-introduce the build hardening that 5459e78e8 attempted, in a form that coexists with Arch/Pikaur builds (the conflict that forced its revert in 0722dc9e3). configure.ac: - Add -D_FORTIFY_SOURCE=2 only when neither $CFLAGS nor $CPPFLAGS already define _FORTIFY_SOURCE (makepkg injects its own) and not under --coverage. - Route glib/gio CFLAGS through -isystem so their macros don't trip our -W set. - Add --enable-sanitizers (ASan + UBSan; unsigned-integer-overflow probed, skipped on gcc) for a dedicated CI job, off by default. - Add --enable-hardening to gate -Wnull-dereference (false positives under -O2 on some toolchains), off by default. - Promote -Wsign-compare from -Wno- to an active probed warning. Also fix the sign-conversion sites that -Wsign-compare/-Wconversion cleanup surfaced as safe (pointer-diff to gsize via new g_diff_to_gsize helper, non-negative int casts), and validate the /vcard remove index through strtoi_range so a negative argument can no longer become a huge unsigned index. Refs #112 --- configure.ac | 44 ++++++++++++++++++++++++++++++++-- src/command/cmd_funcs.c | 10 ++++++-- src/common.c | 15 ++++++------ src/common.h | 8 +++++++ src/database_export.c | 4 ++-- src/database_flatfile.c | 12 +++++----- src/database_flatfile_parser.c | 16 ++++++------- src/profanity.c | 2 +- src/xmpp/jid.c | 4 ++-- src/xmpp/muc.c | 2 +- 10 files changed, 86 insertions(+), 31 deletions(-) diff --git a/configure.ac b/configure.ac index 37034826..dece25a2 100644 --- a/configure.ac +++ b/configure.ac @@ -71,6 +71,10 @@ AC_ARG_ENABLE([omemo-qrcode], [AS_HELP_STRING([--enable-omemo-qrcode], [enable ability to display omemo qr code])]) AC_ARG_ENABLE([coverage], [AS_HELP_STRING([--enable-coverage], [enable code coverage analysis])]) +AC_ARG_ENABLE([sanitizers], + [AS_HELP_STRING([--enable-sanitizers], [enable AddressSanitizer + UndefinedBehaviorSanitizer + unsigned-integer-overflow])]) +AC_ARG_ENABLE([hardening], + [AS_HELP_STRING([--enable-hardening], [enable extra hardening warnings (e.g. -Wnull-dereference) that may produce false positives under -O2])]) m4_include([m4/ax_valgrind_check.m4]) AX_VALGRIND_DFLT([drd], [off]) @@ -395,7 +399,7 @@ AC_SUBST([FORKPTY_LIB]) ## Default parameters AM_CFLAGS="$AM_CFLAGS -Wall -Wextra -Wformat=2 -Wno-format-zero-length" -AM_CFLAGS="$AM_CFLAGS -Wno-deprecated-declarations -Wno-unused-parameter -Wno-missing-field-initializers -Wno-sign-compare -Wno-cast-function-type" +AM_CFLAGS="$AM_CFLAGS -Wno-deprecated-declarations -Wno-unused-parameter -Wno-missing-field-initializers -Wno-cast-function-type" AM_CFLAGS="$AM_CFLAGS -Wpointer-arith" AM_CFLAGS="$AM_CFLAGS -Wimplicit-function-declaration" AM_CFLAGS="$AM_CFLAGS -Wundef" @@ -406,7 +410,8 @@ AM_CFLAGS="$AM_CFLAGS -std=gnu99 -ggdb3" # GCC-specific warnings (not supported by clang) — test each one saved_CFLAGS="$CFLAGS" for _flag in -Wlogical-op -Wduplicated-cond -Wduplicated-branches \ - -Wstringop-overflow -Warray-bounds=2 -Walloc-zero; do + -Wstringop-overflow -Warray-bounds=2 -Walloc-zero \ + -Wsign-compare; do AC_MSG_CHECKING([whether $CC supports $_flag]) CFLAGS="$saved_CFLAGS $_flag -Werror" AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], @@ -433,11 +438,46 @@ AS_IF([test "x$enable_coverage" = xyes], AM_LDFLAGS="$AM_LDFLAGS --coverage" AC_MSG_NOTICE([Code coverage analysis enabled])]) +AS_IF([test "x$enable_sanitizers" = xyes], + [SAN_FLAGS="-fsanitize=address,undefined" + saved_CFLAGS="$CFLAGS" + CFLAGS="$saved_CFLAGS -fsanitize=unsigned-integer-overflow -Werror" + AC_MSG_CHECKING([whether $CC supports -fsanitize=unsigned-integer-overflow]) + AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], + [AC_MSG_RESULT([yes]); SAN_FLAGS="$SAN_FLAGS,unsigned-integer-overflow"], + [AC_MSG_RESULT([no])]) + CFLAGS="$saved_CFLAGS" + AM_CFLAGS="$AM_CFLAGS $SAN_FLAGS -fno-sanitize-recover=all -fno-omit-frame-pointer" + AM_LDFLAGS="$AM_LDFLAGS -fsanitize=address,undefined" + AC_MSG_NOTICE([Sanitizers enabled: $SAN_FLAGS])]) + +AS_IF([test "x$enable_hardening" = xyes], + [saved_CFLAGS="$CFLAGS" + for _flag in -Wnull-dereference; do + AC_MSG_CHECKING([whether $CC supports $_flag]) + CFLAGS="$saved_CFLAGS $_flag -Werror" + AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], + [AC_MSG_RESULT([yes]); AM_CFLAGS="$AM_CFLAGS $_flag"], + [AC_MSG_RESULT([no])]) + done + CFLAGS="$saved_CFLAGS" + AC_MSG_NOTICE([Extra hardening warnings enabled])]) + +# Skip _FORTIFY_SOURCE=2 if env already set one (Pikaur/makepkg) or under coverage -O0 (FORTIFY needs -O>0). +AS_IF([test "x$enable_coverage" != xyes], + [AS_CASE([" ${CPPFLAGS} ${CFLAGS} "], + [*_FORTIFY_SOURCE=*], [AC_MSG_NOTICE([_FORTIFY_SOURCE preset from environment, leaving as-is])], + [AM_CFLAGS="$AM_CFLAGS -D_FORTIFY_SOURCE=2"])]) + AS_IF([test "x$PACKAGE_STATUS" = xdevelopment], [AM_CFLAGS="$AM_CFLAGS -Wunused -Werror"]) AS_IF([test "x$PLATFORM" = xosx], [AM_CFLAGS="$AM_CFLAGS -Qunused-arguments"]) +# Treat glib/gio headers as system to keep their macros out of our -W* set. +glib_CFLAGS=$(echo "$glib_CFLAGS" | sed 's/-I/-isystem /g') +gio_CFLAGS=$(echo "$gio_CFLAGS" | sed 's/-I/-isystem /g') + AM_CFLAGS="$AM_CFLAGS $PTHREAD_CFLAGS $glib_CFLAGS $gio_CFLAGS $curl_CFLAGS ${SQLITE_CFLAGS}" AM_CFLAGS="$AM_CFLAGS $libnotify_CFLAGS ${GTK_CFLAGS} $python_CFLAGS" AM_CFLAGS="$AM_CFLAGS -DTHEMES_PATH=\"\\\"$THEMES_PATH\\\"\" -DICONS_PATH=\"\\\"$ICONS_PATH\\\"\" -DGLOBAL_PYTHON_PLUGINS_PATH=\"\\\"$GLOBAL_PYTHON_PLUGINS_PATH\\\"\" -DGLOBAL_C_PLUGINS_PATH=\"\\\"$GLOBAL_C_PLUGINS_PATH\\\"\"" diff --git a/src/command/cmd_funcs.c b/src/command/cmd_funcs.c index a5ff1eec..827212f1 100644 --- a/src/command/cmd_funcs.c +++ b/src/command/cmd_funcs.c @@ -10063,8 +10063,14 @@ cmd_vcard_remove(ProfWin* window, const char* const command, gchar** args) } if (args[1]) { - vcard_user_remove_element(atoi(args[1])); - cons_show("Removed element at index %d", atoi(args[1])); + int index; + auto_gchar gchar* err_msg = NULL; + if (!strtoi_range(args[1], &index, 0, INT_MAX, &err_msg)) { + cons_show("%s", err_msg); + return TRUE; + } + vcard_user_remove_element((unsigned int)index); + cons_show("Removed element at index %d", index); vcardwin_update(); } else { cons_bad_cmd_usage(command); diff --git a/src/common.c b/src/common.c index 6a0e24fa..bc70b2a3 100644 --- a/src/common.c +++ b/src/common.c @@ -294,7 +294,7 @@ str_replace(const char* string, const char* substr, wr = newstr; while ((tok = strstr(head, substr))) { - size_t l = tok - head; + size_t l = g_diff_to_gsize(tok, head); memcpy(wr, head, l); wr += l; memcpy(wr, replacement, len_replacement); @@ -359,9 +359,9 @@ string_matches_one_of(const char* what, const char* is, gboolean is_can_be_null, char errmsg[256] = { 0 }; size_t sz = 0; int s = snprintf(errmsg, sizeof(errmsg) - sz, "%s must be one of:", what); - if (s < 0 || s + sz >= sizeof(errmsg)) + if (s < 0 || (size_t)s + sz >= sizeof(errmsg)) return ret; - sz += s; + sz += (size_t)s; cur = first; va_start(ap, first); @@ -375,12 +375,12 @@ string_matches_one_of(const char* what, const char* is, gboolean is_can_be_null, errmsg[sz] = '\0'; s = snprintf(errmsg + sz, sizeof(errmsg) - sz, " or '%s'.", cur); } - if (s < 0 || s + sz >= sizeof(errmsg)) { + if (s < 0 || (size_t)s + sz >= sizeof(errmsg)) { log_debug("Error message too long or some other error occurred (%d).", s); s = -1; break; } - sz += s; + sz += (size_t)s; cur = next; } va_end(ap); @@ -431,7 +431,7 @@ str_xml_sanitize(const char* const str) return NULL; } - GString* sanitized = g_string_new_len(NULL, strlen(str)); + GString* sanitized = g_string_new_len(NULL, (gssize)strlen(str)); const char* curr = str; while (*curr != '\0') { @@ -692,12 +692,13 @@ get_file_paths_recursive(const char* path, GSList** contents) gchar* get_random_string(int length) { + g_assert(length >= 0); GRand* prng; gchar* rand; gchar alphabet[] = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"; int endrange = sizeof(alphabet) - 1; - rand = g_malloc0(length + 1); + rand = g_malloc0((gsize)length + 1); prng = g_rand_new(); diff --git a/src/common.h b/src/common.h index 40c68d60..efe89cf9 100644 --- a/src/common.h +++ b/src/common.h @@ -158,6 +158,14 @@ gboolean create_dir(const char* name); gboolean copy_file(const char* const src, const char* const target, const gboolean overwrite_existing); char* str_replace(const char* string, const char* substr, const char* replacement); gboolean strtoi_range(const char* str, int* saveptr, int min, int max, char** err_msg); + +// Pointer-diff to gsize for callers that know end >= start by construction. +static inline gsize +g_diff_to_gsize(const void* end, const void* start) +{ + g_assert(end >= start); + return (gsize)((const char*)end - (const char*)start); +} int utf8_display_len(const char* const str); gchar* str_xml_sanitize(const char* const str); diff --git a/src/database_export.c b/src/database_export.c index 7e21e368..d4954a1f 100644 --- a/src/database_export.c +++ b/src/database_export.c @@ -282,7 +282,7 @@ _export_one_contact(const char* contact) gchar* key = _make_dedup_key(pl->stanza_id, pl->timestamp_str, pl->from_jid, pl->message); g_hash_table_add(seen_keys, key); } - int existing_count = g_slist_length(existing); + int existing_count = (int)g_slist_length(existing); // 2. Query SQLite for this contact — get ALL messages via direct SQL GSList* sqlite_lines = db_sqlite_get_all_chat(contact); @@ -581,7 +581,7 @@ log_database_import_from_flatfile(const gchar* const contact_jid) // Wrap in a transaction for atomicity and performance int contact_imported = 0; int contact_skipped = 0; - int total_lines = g_slist_length(ff_lines); + int total_lines = (int)g_slist_length(ff_lines); db_sqlite_begin_transaction(); gboolean import_ok = TRUE; diff --git a/src/database_flatfile.c b/src/database_flatfile.c index 5c70eb08..8c4222fe 100644 --- a/src/database_flatfile.c +++ b/src/database_flatfile.c @@ -88,7 +88,7 @@ _ff_cache_line_ids(ff_contact_state_t* state, const char* line) return; // Split metadata on unescaped '|' - auto_gchar gchar* meta_str = g_strndup(bracket + 1, close - bracket - 1); + auto_gchar gchar* meta_str = g_strndup(bracket + 1, g_diff_to_gsize(close, bracket + 1)); char** parts = ff_split_meta(meta_str); char* stanza_id = NULL; @@ -111,9 +111,9 @@ _ff_cache_line_ids(ff_contact_state_t* state, const char* line) const char* sender_start = close + 2; const char* colonspace = ff_find_unescaped_colonspace(sender_start); if (colonspace) { - const char* slash = memchr(sender_start, '/', colonspace - sender_start); + const char* slash = memchr(sender_start, '/', g_diff_to_gsize(colonspace, sender_start)); const char* jid_end = slash ? slash : colonspace; - from_jid = g_strndup(sender_start, jid_end - sender_start); + from_jid = g_strndup(sender_start, g_diff_to_gsize(jid_end, sender_start)); } } @@ -140,7 +140,7 @@ _ff_maybe_index_line(ff_contact_state_t* state, const char* buf, off_t pos) if (!space) return; - auto_gchar gchar* ts_str = g_strndup(buf, space - buf); + auto_gchar gchar* ts_str = g_strndup(buf, g_diff_to_gsize(space, buf)); GDateTime* dt = g_date_time_new_from_iso8601(ts_str, NULL); if (!dt) { log_warning("flatfile: unparsable timestamp in %s at offset %ld: %s", @@ -771,7 +771,7 @@ _flatfile_get_previous_chat(const gchar* const contact_barejid, const gchar* sta // For "last N messages": back up from read_to using index if (!from_start) { - int margin_entries = (MESSAGES_TO_RETRIEVE * 3) / FF_INDEX_STEP + 2; + size_t margin_entries = (MESSAGES_TO_RETRIEVE * 3) / FF_INDEX_STEP + 2; // Find index entry closest to (but before) read_to size_t entry_idx = 0; @@ -781,7 +781,7 @@ _flatfile_get_previous_chat(const gchar* const contact_barejid, const gchar* sta entry_idx = i; } - size_t start_entry = entry_idx > (size_t)margin_entries + size_t start_entry = entry_idx > margin_entries ? entry_idx - margin_entries : 0; off_t backed = state->entries[start_entry].byte_offset; diff --git a/src/database_flatfile_parser.c b/src/database_flatfile_parser.c index 932878df..7e6120f2 100644 --- a/src/database_flatfile_parser.c +++ b/src/database_flatfile_parser.c @@ -553,7 +553,7 @@ ff_split_meta(const char* meta) continue; } if (*p == '|' || *p == '\0') { - g_ptr_array_add(arr, g_strndup(start, p - start)); + g_ptr_array_add(arr, g_strndup(start, g_diff_to_gsize(p, start))); if (*p == '\0') break; start = p + 1; @@ -710,7 +710,7 @@ ff_parse_line(const char* line) if (bracket_start && first_space && first_space < bracket_start) { // Standard format with metadata: {timestamp} [{meta}] {sender}: {msg} - result->timestamp_str = g_strndup(work, first_space - work); + result->timestamp_str = g_strndup(work, g_diff_to_gsize(first_space, work)); // Parse metadata section [...] const char* bracket_end = ff_find_unescaped_char(bracket_start + 1, ']'); @@ -721,7 +721,7 @@ ff_parse_line(const char* line) return NULL; } - auto_gchar gchar* meta_content = g_strndup(bracket_start + 1, bracket_end - bracket_start - 1); + auto_gchar gchar* meta_content = g_strndup(bracket_start + 1, g_diff_to_gsize(bracket_end, bracket_start + 1)); char** parts = ff_split_meta(meta_content); if (parts) { _ff_parse_meta_parts(parts, result); @@ -736,12 +736,12 @@ ff_parse_line(const char* line) // Find first *unescaped* ': ' which separates sender from message. const char* colon = ff_find_unescaped_colonspace(after_meta); if (colon) { - auto_gchar gchar* raw_sender = g_strndup(after_meta, colon - after_meta); + auto_gchar gchar* raw_sender = g_strndup(after_meta, g_diff_to_gsize(colon, after_meta)); // Split sender into jid/resource, then unescape resource char* slash = strchr(raw_sender, '/'); if (slash) { - result->from_jid = g_strndup(raw_sender, slash - raw_sender); + result->from_jid = g_strndup(raw_sender, g_diff_to_gsize(slash, raw_sender)); result->from_resource = ff_unescape_sender_resource(slash + 1); } else { result->from_jid = g_strdup(raw_sender); @@ -755,7 +755,7 @@ ff_parse_line(const char* line) } } else if (first_space) { // Legacy/simple format without metadata: {timestamp} - {sender}: {msg} - result->timestamp_str = g_strndup(work, first_space - work); + result->timestamp_str = g_strndup(work, g_diff_to_gsize(first_space, work)); result->type = g_strdup("chat"); result->enc = g_strdup("none"); @@ -767,10 +767,10 @@ ff_parse_line(const char* line) char* colon = strstr(rest, ": "); if (colon) { - char* sender = g_strndup(rest, colon - rest); + char* sender = g_strndup(rest, g_diff_to_gsize(colon, rest)); char* slash = strchr(sender, '/'); if (slash) { - result->from_jid = g_strndup(sender, slash - sender); + result->from_jid = g_strndup(sender, g_diff_to_gsize(slash, sender)); result->from_resource = g_strdup(slash + 1); } else { result->from_jid = g_strdup(sender); diff --git a/src/profanity.c b/src/profanity.c index 08655295..4477d6a9 100644 --- a/src/profanity.c +++ b/src/profanity.c @@ -138,7 +138,7 @@ prof_run(gchar* log_level, gchar* account_name, gchar* config_file, gchar* log_f * so we can be sure there's runtime left after executing * the last command. */ - min_runtime += waittime; + min_runtime += (unsigned int)waittime; } else { log_error("%s", err_msg); g_free(err_msg); diff --git a/src/xmpp/jid.c b/src/xmpp/jid.c index c36538e1..1a191dcf 100644 --- a/src/xmpp/jid.c +++ b/src/xmpp/jid.c @@ -117,7 +117,7 @@ jid_is_valid(const gchar* const str) // Localpart validation if (at) { - size_t local_len = at - str; + size_t local_len = g_diff_to_gsize(at, str); if (local_len == 0 || local_len > JID_MAX_PART_LEN) { return FALSE; } @@ -135,7 +135,7 @@ jid_is_valid(const gchar* const str) // Resourcepart validation if present if (slash) { - domain_len = slash - domain_start; + domain_len = g_diff_to_gsize(slash, domain_start); size_t resource_len = strlen(slash + 1); if (resource_len > JID_MAX_PART_LEN) { return FALSE; diff --git a/src/xmpp/muc.c b/src/xmpp/muc.c index 472c7ee3..da9c925d 100644 --- a/src/xmpp/muc.c +++ b/src/xmpp/muc.c @@ -707,7 +707,7 @@ muc_autocomplete(ProfWin* window, const char* const input, gboolean previous) } else { search_str = last_space + 1; if (!chat_room->autocomplete_prefix) { - chat_room->autocomplete_prefix = g_strndup(input, search_str - input); + chat_room->autocomplete_prefix = g_strndup(input, g_diff_to_gsize(search_str, input)); } }