no-DB mode implementation #94
Reference in New Issue
Block a user
No description provided.
Delete Branch "feat/no-db-mode"
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?
Flat-file database backend (SQLite-free mode)
Adds a pluggable flat-file storage backend for message history, allowing profanity to build and run without the SQLite dependency (
./configure --without-sqlite). Includes bidirectional migration commands and comprehensive test coverage.Architecture
Backend dispatch (
database.c, database.h)The monolithic
database.chas been refactored into a vtable-based dispatch layer. A singledb_backend_tpointer (active_db_backend) holds the active implementation. All publiclog_database_*()functions are thin wrappers that forward to the active backend's function pointers:log_database_init()readsPREF_DBLOGto select the backend;log_database_switch_backend()closes the current one, persists the preference, and reinitializes with the active account — requiring an active XMPP session to resolve the storage path.SQLite backend (
database_sqlite.c— 965 lines)Extracted from the old
database.cwith no behavioral changes. Schema usesChatLogstable with dual-FK LMC tracking (replaces_db_id↔replaced_by_db_id) and anAFTER INSERTtrigger to maintain the back-reference. Indexes on(to_jid, from_jid)andtimestampfor range queries.Flat-file backend (
database_flatfile.c— 1039 lines)One UTF-8 text file per contact in
flatlog/<account>/<jid_dir>/chat.log. Each line is a single message with pipe-delimited metadata:Key data structures per contact (
ff_contact_state_t):FF_INDEX_STEP), storing byte offset + epoch timestamp. Enables O(log n) binary search for time-range queries on a typical 10K-message contact with only ~20 index entries in memory.archive_idshash set: O(1) MAM dedup — prevents duplicate archive_id on merge/import.stanza_sendershash map: Maps stanza_id → from_jid for O(1) LMC sender validation (rejects corrections from a different JID than the original sender)._ff_cache_line_ids()): Lightweight pass that extracts stanza_id, archive_id, and from_jid without full parsing (no GDateTime allocation, no unescape) — used during index build.Parser (
database_flatfile_parser.c— 780 lines)Handles escaping, UTF-8 validation, and legacy format support:
\n,\r,\\) and metadata values (additionally\|,\]) — chosen over base64 to preservegrep-ability.": "(colon-space) to prevent false message boundary detection.chatlog.cfiles.Integrity checker (
database_flatfile_verify.c— 309 lines)Multi-level verification returning
GSList<integrity_issue_t>with severity:ff_parse_line()round-tripExport/import engine (
database_export.c— 559 lines,HAVE_SQLITEonly)Bidirectional migration between SQLite and flat-file with merge-sort deduplication:
timestamp|from_jid|body_prefixflock()+ rename)Contact discovery reverses the
@→_at_JID mapping from the flatlog directory structure.Security hardening
ff_jid_to_dir()replaces/,\with_; collapses..to__; rejects empty results.ff_ensure_dir()checks final path component against symlinks viag_lstat().\n,\r,|,],\all backslash-escaped. Prevents crafted stanza_id from injecting fake log lines.stanza_sendersmap — rejects corrections from different JID than original sender.User-facing commands
/history switch sqlite|flatfile/history export [jid]/history import [jid]/history verify [jid]Build system
./configure --without-sqlite— flat-file only, no SQLite dependencyHAVE_SQLITE/BUILD_SQLITEconditionals gate export/import and SQLite backendTest coverage (4300+ new test lines)
129 functional tests in 4 balanced parallel groups (~50-53s each):
/time chat set iso8601), MUC groupchat, non-UTC timezone, deduplication, negative assertions/history on— previously passed by accident due to cumulative PTY buffer matching483 unit tests including:
Test infrastructure: per-test
clock_gettime(CLOCK_MONOTONIC)timing with configurable slow/fast thresholds,NEGATIVE_ASSERT_TIMEOUTfor absence verification,prof_test_tzoverride for timezone tests.Performance
send_receipt_requestfunctional test: 60.7s → 1.1s (sha-256 → sha-1 for stanza-id matching in stabber)g_slist_append()→g_slist_prepend()+g_slist_reverse()throughout (O(n²) → O(n))Files changed
database_flatfile.c,_parser.c,_verify.c,.hdatabase_sqlite.cdatabase_export.cdatabase.c, database.hcmd_funcs.c,cmd_defs.c,cmd_ac.ctest_history.c, functionaltests.ctest_database_export.c,test_database_stress.ccloses #48
fixes #48
resolves #48
Add pluggable storage backend abstraction (vtable) to the database layer, allowing selection between SQLite (default) and a new flat-file backend that stores messages as human-readable plain text files. New files: - database_sqlite.c: extracted SQLite backend from database.c - database_flatfile.c: plain text backend with tolerant parser, LMC correction chains, UTF-8/BOM/CRLF handling, integrity checks Commands: - /privacy logging flatfile — switch to flat-file backend - /history verify [<jid>] — check integrity of stored history Fixes: - Add missing 'off' guard in flatfile backend - Enable CHLOG/HISTORY prefs when switching to flatfile mode Logs stored in ~/.local/share/profanity/flatlog/{account}/{contact}/{date}.log Format: {ISO8601} [{type}|{enc}|id:{id}|corrects:{id}] {sender}: {message} Updated: CHANGELOG, CONTRIBUTING.md, profrc.example, man page, cmd_defs, Makefile.am, test stubs, functional test support (PROF_FLATFILE=1)Security fixes for 7 vulnerabilities in database_flatfile.c: 1. Path traversal via crafted JID (HIGH): _ff_jid_to_dir() now strips '/', '\' -> '_' and collapses '..' -> '__', preventing a malicious federated JID from escaping the log directory. 2. Log injection via unescaped message body (HIGH): Add _ff_escape_message()/_ff_unescape_message() -- escape \n, \r, \\ in message text on write, unescape on read. Prevents remote contacts from injecting fake log lines with forged sender/timestamp/encryption. 3. Metadata field injection (HIGH): Add _ff_escape_meta_value()/_ff_unescape_meta_value() -- escape |, ], \\, \n, \r in stanza_id/archive_id/replace_id. Parser uses escape-aware _ff_find_unescaped_char() and _ff_split_meta() instead of strchr/strsplit. 4. Sender ": " parsing confusion (MEDIUM): Escape ": " -> "\: " in XMPP resource on write. Parser uses _ff_find_unescaped_colonspace() + _ff_unescape_sender_resource(). 5. LMC correction chain cycle -> infinite loop (MEDIUM): Add visited hash-set and FF_MAX_LMC_DEPTH (100) limit to chain walker. 6. Unbounded getline() -> OOM (MEDIUM): Add FF_MAX_LINE_LEN (10 MB) cap in _ff_readline(); overlength lines are skipped with a warning. Replaces all char buf[8192]/fgets() sites with dynamic getline() + truncated-line detection at EOF. 7. Symlink attack + TOCTOU on file creation (MEDIUM): Replace fopen("a") with open(O_WRONLY|O_APPEND|O_CREAT|O_EXCL|O_NOFOLLOW) + fdopen() -- atomic new-file detection, symlink rejection via O_NOFOLLOW. _ff_ensure_dir() checks g_lstat() for symlinks. File permissions 0600 set via open() mode, not post-hoc g_chmod().ba515bdfe8to32fe23474432fe234744to6f8832377aBenchmark: daily files vs single file storage
Test setup:
Two layouts compared:
{contact}/{YYYY-MM-DD}.log— 522 files, ~60 KB each{contact}.log— 1 file, 30 MBMethodology:
_flatfile_get_previous_chat(): file listing → read & parse → LMC correction → pagination (last 51 messages)posix_fadvise(DONTNEED)evicts page cache before each read — measures real disk I/O on NVMesleep()for 7200 RPM characteristics (8 ms seek, 4.17 ms rotational, 150 MB/s sequential). Fragmentation modeled as extra seek per 64 KB fragment boundaryResults (best of N runs):
Conclusions:
get_previous_chat()always passes a time range1b349fecb5to59fe15f4d0Build system: - Add --without-sqlite configure flag (AM_CONDITIONAL BUILD_SQLITE) - Guard database_sqlite.c with HAVE_SQLITE in Makefile.am, database.c, database.h and stub_database.c - Fall back to flatfile when SQLite not compiled in Security (database_flatfile.c): - MAM dedup: skip incoming messages with duplicate stanza-id (archive_id) - LMC sender validation: reject corrections from mismatched JIDs - Add flock() advisory locking to prevent interleaved writes from concurrent instances - Check fprintf return when writing file header Autocomplete (cmd_ac.c): - Add 'flatfile' to /privacy logging autocomplete - Add dedicated /history autocomplete with on/off/verify (was boolean-only) Code quality: - Fix fwrite return type: size_t not ssize_t (database_flatfile_parser.c) - Fix mixed allocators in ff_readline: use malloc() consistently for overlength-line fallback instead of g_strdup() - Fix index alignment in _ff_state_extend: use local counter so index step doesn't depend on total_lines from initial build Documentation: - Fix page: correct directory structure (history.log not per-day files) - Fix page: add aid:{archive_id} to line format example - Fix page: missing newline before .SH BUGS8868b79617to0f9157bd3a650686c241to6f543e124d081de77593to9183ec7c2afa2b21152fto16476676f00274f52943to0d38a0a2cbWhile the code has a lot of merits, certain parts should be adjusted to follow the best practices, as well as to correct logic. Specific parts would require further discussion.
@@ -188,3 +197,2 @@tests/functionaltests/test_lastactivity.c tests/functionaltests/test_lastactivity.h \tests/functionaltests/test_autoping.c tests/functionaltests/test_autoping.h \tests/functionaltests/test_disco.c tests/functionaltests/test_disco.h \tests/functionaltests/test_autoping.c tests/functionaltests/test_autoping.h \Is it a wrong offset or is it gitea's UI bug? It appears that the line is overly offset to the right, violating general formatting
Corrected
@@ -97,2 +97,2 @@PKG_CHECK_MODULES([SQLITE], [sqlite3 >= 3.22.0], [],[AC_MSG_ERROR([sqlite3 3.22.0 or higher is required])])### sqlite (optional — can be disabled with --without-sqlite)AC_ARG_WITH([sqlite],General note (I am checking all the changed from top to bottom, this change is on the top): is this option supported by the code: does it switch by default to flatfile if sqlite is not available; does it prevent user from switching to sqlite manually.
@@ -198,3 +198,3 @@is stored in.I $XDG_CONFIG_HOME/profanity/profrc, details on commands for configuring Profanity can be found at <https://profanity-im.github.io/reference.html> or the respective built\-in help or man pages., details on commands for configuring Profanity can be found at <https://profanity-im.github.io/reference.html> or the respective built\-in help or man pages..SS Message History StorageSince the line is changed, it can also lead to the docs on our website, https://jabber.space/command-reference/
Also, why ".SS" is on the same page?
.SS fixed and link to jabber.space/command-reference/ added
@@ -49,6 +49,13 @@ grlog=truemaxsize=1048580rotate=trueshared=true# Database backend for message history:While the comment is useful, it violates general style of this example. I suggest moving documentation to more appropriate places.
example simplified, the long comment removed; explanation lives in the man page now
@@ -1888,2 +1896,3 @@CMD_ARGS({ "on|off", "Enable or disable showing chat history." }){ "on|off", "Enable or disable showing chat history." },{ "switch sqlite|flatfile", "Switch the active database backend at runtime." },Does the switch automatically export/import? It is unclear from documentation.
documentation-only clarification, queued. /history switch only swaps the active backend; data is not migrated unless the user runs /history export and /history import explicitly.
@@ -1889,1 +1897,3 @@{ "on|off", "Enable or disable showing chat history." }){ "on|off", "Enable or disable showing chat history." },{ "switch sqlite|flatfile", "Switch the active database backend at runtime." },{ "verify [<jid>]", "Verify integrity of message history. Optionally specify a JID to check only one contact." },How does it align with flatfile manual editability?
needs a doc paragraph explaining that ff_verify_integrity flags timestamp drift, dup IDs, broken LMC refs etc. so manual edits remain auditable. Doc-only follow-up
@@ -2719,3 +2731,3 @@CMD_TAG_DISCOVERY)CMD_SYN("/privacy logging on|redact|off","/privacy logging on|redact|off|flatfile",on|flatfile|redact|offformat would look more consistent (from most logging to least logging).Additionally, necessity for the
redactoption needs to be discussed. Though, as far as I remember, it is necessary for OMEMO/groupchat XEPs to properly sync the chats.design discussion. Listing order is cosmetic; the redact mode question is a separate decision. No code change in this PR
@@ -6687,2 +6687,4 @@cons_show("Messages are going to be redacted.");prefs_set_string(PREF_DBLOG, arg);} else if (g_strcmp0(arg, "flatfile") == 0) {cons_show("Using flat-file backend for message logging. Takes effect on next connection.");It might be beneficial to also output flatfile location to the user.
Corrected
@@ -6733,0 +6738,4 @@if (g_strcmp0(args[0], "switch") == 0) {jabber_conn_status_t conn_status = connection_get_status();if (conn_status != JABBER_CONNECTED) {cons_show_error("You must be connected to switch database backend.");It future, this requirement should be removed as it doesn't make much sense. Offline capabilities are really lacking. But I see why it exists (local SQLite is getting disconnected on disconnect).
Feel free to click "resolve" on this comment.
@@ -6733,0 +6747,4 @@return TRUE;}if (g_strcmp0(args[1], "sqlite") != 0 && g_strcmp0(args[1], "flatfile") != 0) {Nice nesting: everything is flat. Easy to read, easy to maintain.
Resolveable.
@@ -6733,0 +6795,4 @@}if (issue->line > 0) {if (issue->level == INTEGRITY_ERROR) {cons_show_error("[%s] %s:%d — %s", level_str, issue->file, issue->line, issue->message);nit: It might be more readable if it's in the separate method. I suggest extraction of the error printing in a separate method.
Corrected
@@ -252,0 +75,4 @@active_db_backend = db_backend_sqlite();log_info("Using SQLite database backend");#elselog_info("SQLite not compiled in, falling back to flat-file backend");Shouldn't it be a warning, since it's not an expected state?
Corrected
@@ -262,0 +105,4 @@// Update preferenceprefs_set_string(PREF_DBLOG, new_backend);prefs_save();Typically, prefs are not autosaved and need manual saving via
/savecommand. This place creates an exception, which needs to be discussed (e.g., later we might add/autosavecommand).Corrected
@@ -269,2 +131,2 @@} else {_add_to_db(message, NULL, message->from_jid, connection_get_jid());if (active_db_backend && active_db_backend->add_incoming) {active_db_backend->add_incoming(message);here and in other places: if the method does not exists, then it just passes. I think that we need to log this invalid state.
Corrected
@@ -0,0 +2,4 @@* database_export.c* vim: expandtab:ts=4:sts=4:sw=4** Copyright (C) 2026 Profanity ContributorsWe are not affiliated with Profanity.
Use the following lines for copyright notice:
Corrected
@@ -0,0 +106,4 @@GDir* dir = g_dir_open(base_dir, 0, &err);if (!dir) {if (err) {log_debug("export: cannot open flatlog dir %s: %s", base_dir, err->message);the issue doesn't appear to be pure
debuglevel, but ratherwarnat the very leastCorrected
@@ -0,0 +109,4 @@log_debug("export: cannot open flatlog dir %s: %s", base_dir, err->message);g_error_free(err);}return NULL;will fail silently if no
erris present.Corrected
@@ -0,0 +146,4 @@while (1) {gboolean truncated = FALSE;char* buf = ff_readline(fp, &truncated);why not
auto_charinstead offree'ing?Corrected
@@ -0,0 +334,4 @@char* key = _make_dedup_key(sid, ts, from_jid, body);if (g_hash_table_contains(seen_keys, key)) {g_free(key);why
g_freeifkeyischar? Normally, we do this:g_freeforgcharfreeforcharCorrected
@@ -0,0 +387,4 @@if (rename(tmp_path, log_path) != 0) {log_error("export: rename %s -> %s failed: %s", tmp_path, log_path, strerror(errno));unlink(tmp_path);}We can't just print
Exported %s... to user after this case. It's a error and should be handled as such.Corrected
@@ -0,0 +401,4 @@g_slist_free_full(contacts, g_free);return total_exported;}nit: This method is quite fat. In some sense, it's needed for coherency, but I might've overlooked some potential issues with early returns. If possible, I suggest extracting at least body of the cycle.
Corrected
@@ -0,0 +480,4 @@if (!pl || !pl->timestamp)continue;auto_gchar gchar* ts = g_date_time_format_iso8601(pl->timestamp);nit: this conversion is done twice (see l463:
auto_gchar gchar* ts = msg->timestamp ? g_date_time_format_iso8601(msg->timestamp) : g_strdup("");).Postponed — investigated: the two ts conversions on the SAME message happen in the import path (once for dedup-key building, once inside _add_to_db when serializing for SQL). Removing the duplicate would require an _add_to_db API change to accept a pre-formatted ts. Microsecond per row × 1M = ~1 s saving — accepted as deferred until that signature changes for other reasons.
@@ -0,0 +520,4 @@}}sqlite_be->add_incoming(msg);note to check: do we sort by timestamp when we fetch from the sqlite? Or is it default sorting by id which presumes that all messages are in order?
Verified — no change needed. db_sqlite_get_all_chat already has "ORDER BY A.timestamp ASC" (database_sqlite.c:916). Documented in
555faaa23commit body.@@ -0,0 +531,4 @@}contact_imported++;if (contact_imported % 500 == 0) {magic number
Corrected
@@ -0,0 +546,4 @@cons_show("Imported %s: %d new, %d skipped (already in SQLite)",contact, contact_imported, contact_skipped);total_imported += contact_imported;total_skipped += contact_skipped;total_skippedis unused variableCorrected
@@ -0,0 +2,4 @@* database_flatfile.c* vim: expandtab:ts=4:sts=4:sw=4** Copyright (C) 2026 Profanity ContributorsSame issue as earlier with copyright. We are not affiliated with Profanity.
Use the following lines for copyright notice:
Same issue with other files.
Corrected
@@ -0,0 +86,4 @@g_free(state);}// Lightweight ID extraction from a raw log line for the dedup/LMC cache.general note: LMC is an optional feature. If it's disabled and the log is refetched, it should not be applied.
Corrected
@@ -0,0 +115,4 @@} else if (g_str_has_prefix(parts[i], "aid:") && !archive_id) {archive_id = ff_unescape_meta_value(parts[i] + 4);}}nit: I suggest this version for readability (avoid magic number of 3 and 4).
Ideally, we can also move "id:" and "aid:" into separate constants. It would also increase robustness in case of prefix changes.
Corrected
@@ -0,0 +121,4 @@// Extract from_jid (needed for stanza_senders map): after "] " before "/" or ": "char* from_jid = NULL;if (stanza_id && *(close + 1) == ' ') {what if
*(close + 1)overflows?Corrected
@@ -0,0 +122,4 @@// Extract from_jid (needed for stanza_senders map): after "] " before "/" or ": "char* from_jid = NULL;if (stanza_id && *(close + 1) == ' ') {const char* sender_start = close + 2;again no overflow check
Corrected
@@ -0,0 +126,4 @@const char* colonspace = ff_find_unescaped_colonspace(sender_start);if (colonspace) {const char* jid_end = colonspace;for (const char* p = sender_start; p < colonspace; p++) {Instead of loop, we can do something like that:
g_utf8_strchr(sender_start, colonspace, '/')@@ -0,0 +160,4 @@int c1 = fgetc(fp);int c2 = fgetc(fp);int c3 = fgetc(fp);if (c1 == 0xEF && c2 == 0xBB && c3 == 0xBF) {Please, extract bom-check to a different method. it is violates DRY and SRP.
database_flatfile_validate.c:123Corrected
@@ -0,0 +196,4 @@// Extract IDs for O(1) dedup/LMC cache_ff_cache_line_ids(state, buf);if (msg_count % FF_INDEX_STEP == 0) {please add comment explaining purpose
Corrected
@@ -0,0 +201,4 @@if (space) {char* ts_str = g_strndup(buf, space - buf);GDateTime* dt = g_date_time_new_from_iso8601(ts_str, NULL);if (dt) {Are
!dtand!spacecases ok to be ignored?Postponed — discussion. Current behaviour is "skip silently" (lines outside index step are not indexed); this is by design. No specific change requested.
@@ -0,0 +203,4 @@GDateTime* dt = g_date_time_new_from_iso8601(ts_str, NULL);if (dt) {if (state->n_entries >= state->cap_entries) {state->cap_entries = state->cap_entries ? state->cap_entries * 2 : 64;please, consider reduction of nesting
Corrected
@@ -0,0 +283,4 @@// Extract IDs for O(1) dedup/LMC cache_ff_cache_line_ids(state, buf);if (new_count % FF_INDEX_STEP == 0) {the logic seemingly mirrors the one from
_ff_state_build, creating repetition. I suggest extracting it to a different method to keep things DRYer and more maintainableCorrected
@@ -0,0 +405,4 @@{auto_gchar gchar* pref_dblog = prefs_get_string(PREF_DBLOG);if (g_strcmp0(pref_dblog, "off") == 0) {note to check: is it case sensitive/get forced lowercase when set by user?
Checked cmd_funcs.c:6676:
resolved
@@ -0,0 +448,4 @@// Open the file with O_NOFOLLOW to prevent symlink attacks,// and O_APPEND for safe concurrent appends.// Try O_CREAT|O_EXCL first to detect new files atomically (no TOCTOU).int fd = open(log_path, O_WRONLY | O_APPEND | O_CREAT | O_EXCL | O_NOFOLLOW,O_APPENDwon't execute sinceO_EXCLwould throw an error if file already exists.Corrected
@@ -0,0 +591,4 @@return;const Jid* to_jid = message->to_jid ? message->to_jid : connection_get_jid();const char* type = ff_get_message_type_str(message->type);note: is it null safe?
Verified — null-safe by construction. Input is a prof_msg_type_t enum (cannot be NULL), and every switch arm returns a string literal; the catch-all
return "chat"covers any future enum addition. Callers' g_strdup(...) is therefore always safe.@@ -0,0 +604,4 @@// MAM dedup: skip if this stanza-id already exists in the log (XEP-0313 replay)if (message->stanzaid && !message->is_mam) {if (_ff_has_archive_id(contact, message->stanzaid)) {log_error("flatfile: duplicate stanza-id '%s' from %s, skipping",stanza-idcan be non-unique on certain clients (e.g., older versions of Profanity, as well as Pidgin, Adium and otherlibpurple-based XMPP clients which use(d) incremental stanza IDs). It would be more correct to ignore it.See https://github.com/profanity-im/profanity/issues/1899
However,
message->stanzaidlinks to the archive ID, which might be logical in this case.This lines copies functionality of original
database.cand shouldn't cause issues.@@ -0,0 +666,4 @@// Apply LMC: for messages with corrects:X, find original and replace its textstatic void_ff_apply_lmc(GSList* parsed_lines, GSList** result)we should be careful with LMC, since it can be disabled by user, so this part needs further review and discussion.
Corrected
@@ -0,0 +2,4 @@* database_flatfile.h* vim: expandtab:ts=4:sts=4:sw=4** Copyright (C) 2026 Profanity ContributorsSame issue as earlier with copyright. We are not affiliated with Profanity.
Use the following lines for copyright notice:
Corrected
@@ -0,0 +120,4 @@int c1 = fgetc(fp);int c2 = fgetc(fp);int c3 = fgetc(fp);if (c1 == 0xEF && c2 == 0xBB && c3 == 0xBF) {Please, extract bom-check to a different method. it is violates DRY and SRP. Repeated in
database_flatfile.c:163Corrected
@@ -0,0 +114,4 @@// This prevents a malicious federated JID like "../../../tmp/pwned" from// escaping the log directory tree.char*ff_jid_to_dir(const char* jid)I don't understand the need for
gstringandstep1. I propose a simpler solution that might be more efficient, but I might've missed some intricacies:Corrected
Generally, it's good and well-written. Some parts require minor polish + merge to upstream. Overall, it's a solid job.
@@ -0,0 +424,4 @@// Determine which JID to use for the log file path (the "other" party)const char* contact = NULL;const Jid* myjid = connection_get_jid();if (myjid && myjid->barejid && g_strcmp0(from_barejid, myjid->barejid) == 0) {nit: for readability, maybe we can do something like that?
Corrected
@@ -0,0 +187,4 @@if (g_file_test(path, G_FILE_TEST_IS_DIR)) {// Verify it's not a symlinkstruct stat st;if (g_lstat(path, &st) == 0 && S_ISLNK(st.st_mode)) {why not use
g_file_testin a similar fashion here?g_file_test(path, G_FILE_TEST_IS_SYMLINK)Corrected
@@ -0,0 +308,4 @@return NULL;GString* out = g_string_sized_new(strlen(val));for (const char* p = val; *p; p++) {if (*p == '\\' && *(p + 1)) {early exit here would lower the nesting and potentially improve readability
something like:
if(!condition) {
g_string_append_c(out, *p);
continue;
}
Corrected
@@ -0,0 +346,4 @@// newline, or NULL on EOF. Sets *truncated=TRUE if the line had no trailing// newline at EOF (partial write detection).char*ff_readline(FILE* fp, gboolean* truncated)note for myself: come back to it and rereview
@@ -0,0 +371,4 @@*truncated = FALSE;// Return empty string so caller's loop continues (parse will reject it).// Use malloc() so callers can always use free() consistently.char* empty = malloc(1);nit: Why not
strdup("")?Corrected
@@ -0,0 +459,4 @@}// Escape message body to prevent log injectionchar* safe_msg = ff_escape_message(message_text);why not auto_char?
Corrected
@@ -0,0 +470,4 @@size_t written = fwrite(full_line->str, 1, to_write, fp);if (written != to_write) {log_error("flatfile: partial write (%zu/%zu)", written, to_write);}I feel that it's overly complicated. Starting from the
GString* full_line, everything could be done in nearly a one-liner:Corrected
@@ -0,0 +646,4 @@// Parse metadata section [...]const char* bracket_end = ff_find_unescaped_char(bracket_start + 1, ']');if (bracket_end) {please make early return here to significantly reduce nesting
Corrected
@@ -0,0 +652,4 @@// Split by unescaped '|'char** parts = ff_split_meta(meta_content);if (parts) {int i = 0;it could be extracted to another method
Corrected
@@ -0,0 +38,4 @@#include "database_flatfile.h"GSList*ff_verify_integrity(const gchar* const contact_barejid)It's a really big function that should ideally be split for readability. Additionally, I'd like to see at least a short comment describing what's going on. Comment with higher level of abstraction in
.hfile and comment with more details in.cfile so people that are reading header can understand how to use the function, while people that are reading (and likely prepare to edit) source code understand internals.Corrected
@@ -0,0 +43,4 @@GSList* issues = NULL;if (!g_flatfile_account_jid) {integrity_issue_t* issue = g_malloc0(sizeof(integrity_issue_t));Here and everywhere else, we can use new style for consistency with
g_new0Corrected
@@ -0,0 +95,4 @@auto_gchar gchar* filepath = g_strdup_printf("%s/history.log", cdir_path);if (!g_file_test(filepath, G_FILE_TEST_EXISTS))continue;I am not sure if it's ok to just silently
continuein this place. It deserves at the very least debug log.Corrected
@@ -0,0 +134,4 @@char* buf = NULL;int lineno = 0;GDateTime* prev_ts = NULL;GHashTable* seen_ids = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, NULL);I am not sure if the overlap between
stanza_idsandarchive_idsis a valid approach. We might want to separate them in two distinct tables.Postponed, architecture only question
@@ -0,0 +264,4 @@int b1 = fgetc(fp);int b2 = fgetc(fp);int b3 = fgetc(fp);if (!(b1 == 0xEF && b2 == 0xBB && b3 == 0xBF))Please, extract bom-check to a different method.
Corrected
@@ -0,0 +1,966 @@/*It seemingly mirrors functionality of the original
database.c+ adds integrity verification.The main request here is to add comments for readability and comprehension. Otherwise, if the functionality mirrors previous one, so there shouldn't be an issue.
NOTE: we'll need to add migration and new changes after upstream merge.
Corrected
@@ -0,0 +128,4 @@auto_char char* filename = _get_db_filename(account);if (!filename) {sqlite3_shutdown();We should add error logging here.
Corrected
@@ -0,0 +144,4 @@int db_version = _get_db_version();if (db_version == latest_version) {return TRUE;We might want to also check integrity of the tables (whether they exist + maybe even their columns). But it would introduce further complexity and should be a subject of a separate issue.
Postponed
@@ -0,0 +147,4 @@return TRUE;}const char* query = "CREATE TABLE IF NOT EXISTS `ChatLogs` ("Here and in other places, can we return necessary documentation detailing DB fields, decisions, and other useful points?
Corrected
@@ -0,0 +7,4 @@** These tests only exercise the public log_database API through normal* profanity UI operations, so they work with any storage backend.*/Considerations: we need to test for the messages from the same user, but with different resources.
Corrected
@@ -0,0 +1,721 @@/*Add invalid date tests
Corrected
@@ -0,0 +1104,4 @@int corrections_found = 0;while (1) {char* buf = ff_readline(fp, NULL);Maybe
auto_char, ifcommon.himport is allowed?Corrected
@@ -0,0 +1107,4 @@char* buf = ff_readline(fp, NULL);if (!buf)break;if (buf[0] == '#' || buf[0] == '\0') {Here and everywhere else: no need for this check, as it will already be done within the
ff_parse_line.Corrected
Parser (src/database_flatfile_parser.c): - ff_jid_to_dir: drop GString+step1, in-place mutate str_replace result - ff_ensure_dir: g_file_test(G_FILE_TEST_IS_SYMLINK) instead of g_lstat - ff_unescape_*: early-continue for non-escape branch (less nesting) - ff_readline: strdup("") instead of malloc(1)+'\0' for skip-line return - ff_write_line: auto_char safe_msg + single fprintf (drop GString full_line) - ff_parse_line: early return on missing closing bracket; extract metadata-parts loop into _ff_parse_meta_parts helper Verify (src/database_flatfile_verify.c): - Split monolithic ff_verify_integrity into _collect_contact_dirs, _check_permissions, _first_pass, _lmc_pass, _verify_contact_dir - _issue_new constructor; g_new0 throughout - Separate seen_stanza_ids and seen_archive_ids tables (no cross-kind duplicate confusion) - ff_skip_bom in second pass (drop manual fgetc x3) - log_debug when skipping a contact dir without history.log - Doc comment on ff_verify_integrity in header SQLite (src/database_sqlite.c): - Doc block on ChatLogs schema (replaces_db_id <-> replaced_by_db_id invariant maintained by AFTER INSERT trigger) - log_error when _get_db_filename returns NULL during init Flatfile dispatch (src/database_flatfile.c): - Replace if/else with is_outgoing ternary for log-path contact pick Tests: - 6 new invalid-date parser tests: empty / partial ISO / garbage / impossible calendar / negative year / far future timestamp - New functional test message_db_history_multi_resource verifying same-JID-different-resource history rehydration preserves resources - test_database_stress.c LMC chain loop: auto_char buf, drop redundant comment/empty pre-filter (ff_parse_line handles those)End-to-end performance and correctness harness for the flat-file + SQLite database backends. Lives in tests/bench/, built only on demand (`make bench`); not part of `make check`. Components gen_history (P1) Deterministic corpus generator. Knobs: lines, contacts, years, seed, stanza-id mode (uuid/libpurple/conversations/mixed), LMC rate, MAM-OOO rate, resources/contact, length profile (short/mixed/long/extreme). Emits the canonical flatlog/<account>/<contact>/history.log layout used by ff_verify_integrity. ~340 LOC. bench_runner (P1, P2.5) S1 cold tail-access via sparse index S2 warm tail-access (page cache hot) S3 deep pagination (1000 binary-search lookups) S4 first-time index build (cold file -> ff_state_ensure_fresh) S5 incremental extend (asserts no full rebuild path) S6 real ff_verify_integrity over the contact tree Reports total/err/warn/info issue counts in the CSV note. bench_long_messages (P2) L1-L14 long-message stress: 1KB up to 9.9MB bodies, plus oversized line rejection (10MB+1 -> ff_readline returns ""), embedded-newline / pipe / emoji body patterns, full parse on 100x1MB, 1000x100KB sustained append. bench_failure_modes (P3) F1-F15 failure-injection: truncated last line, mid-file CRLF, mid-file BOM, LMC cycle, LMC depth>FF_MAX_LMC_DEPTH, manual ': ' in resource, RTL/ZWSP, Latin-1 byte, empty body, mtime/inode flip, empty file. Each test asserts expected issue levels and reports PASS/FAIL. bench_export_import (P5) Links real database_export.c + database_sqlite.c + database.c and drives log_database_export_to_flatfile / log_database_import_from_flatfile under load. Subcommands: seed, export, import, roundtrip, verify. S7a/b export, S8a/b import, S8e roundtrip with full byte-by-byte content diff of every row in (from_jid, to_jid, message, timestamp, type, stanza_id, archive_id, encryption, replace_id). Make targets bench-quick / bench / bench-full bench-longmsg, bench-failure bench-multicontact, bench-lmc, bench-ooo bench-export, bench-import, bench-roundtrip bench-pipeline, bench-pipeline-max (1M rows) bench-compare, bench-update-baseline Volume controls: BENCH_VOLUME (small/medium/max), BENCH_PIPE_ROWS, BENCH_PIPE_ROWS_MAX, BENCH_DATA_DIR, BENCH_CSV. Baseline + regression checking (P4) tests/bench/baseline.csv 51 rows: S1-S6 x {small,lmc,ooo} + L1-L14 + F1-F15 (11 of 15) + S7/S8 x {pipe100k, pipe1M}. compare_baseline.py median over duplicate rows; exits 1 on any (scenario, volume) slowdown >= threshold (default 25%). Verified at scale bench-pipeline-max: 1,000,000 rows, full content diff seed 17 s, export 304 s, import 31 s, idempotent re-import 10 s, diff 2.7 s -- mismatches=0. Findings surfaced by the harness Export scales super-linearly: 4 s @ 100k -> 304 s @ 1M (76x for 10x rows). Cause: g_slist_sort on the merged list + per-row ProfMessage/ff_parsed_line_t allocations. RSS peaks at 1.4 GB on 1M. Worth a follow-up. Export progress reporting only fires during the write phase -- the merge+sort phase (~95% of wall time at 1M) is silent. /history export and /history import are blocking on the main UI thread; profanity is frozen for the duration (~5 min @ 1M). ff_readline sets *truncated=TRUE on partial-write tail but ff_verify_integrity does not surface this -- partial writes go unflagged (failure-injection F1). Parser silently truncates body at first unescaped ': ' if a resource was manually edited to contain it (F9). In gen_history (caught by the bench's own real-verify pass): g_strndup mid-codepoint truncation on UTF-8 bank strings -- fixed. Linkage strategy database_flatfile.c + parser + verify + common.c are linked unconditionally. The export/import bench additionally links database.c + database_sqlite.c + database_export.c. bench_stubs.c provides minimal stubs for log_*, prefs_*, connection_get_jid, jid_create, files_*, message_*, ui hooks. integrity_issue_free is a weak symbol so it falls back to the real database.c implementation when that file is linked.Address 6 of 7 high/medium audit findings on database_export.c. End-to-end migration of 1M rows: ~5 min -> ~1 min. H1 cache g_slist_length(merged) out of the write loop — was O(n²) via 2000 list walks at 500-row progress interval, now O(1). S7a_export_cold @ 1M: 309 s -> 11.2 s (-96 %) L1 splice existing flatfile lines into merged via ownership transfer instead of 12-strdup deep-copy + g_date_time_ref; free list nodes only, NULL out existing. S7b_export_dedup @ 1M: 304 s -> 13.3 s (-96 %) RSS @ S7b 1M: 2.4 GB -> 1.9 GB H2 mkstemp() with random suffix instead of fixed "{log_path}.export.tmp". Eliminates the unlink-and-retry race where two concurrent exports could clobber each other's in-flight tmp files. M4 SHA-256 over full body (incremental g_checksum_update) in the dedup-key fallback. Previous body[:256] hash collided on common preambles (signatures, code blocks) — second occurrence silently dropped on import. M2 Refuse _ff_read_all_lines on files > 2 GB. Each line expands ~10x in heap on parse; without a cap we OOM-kill prof on large flatfiles. Surface a clear error instead. M5 Break the outer import loop on system-level INSERT failure. Was cascading "Import of X failed" through every remaining contact when the real issue was disk-full / sqlite locked. Verified on bench-pipeline-max: 1M rows, full content diff, mismatches=0. baseline.csv updated. Deferred: M3 O_TMPFILE (platform-conditional + linkat dance; mkstemp covers most of the same window) and M6 SIGINT cancellation (needs event-loop architecture changes).92f601d2eato1fe3808b23LGTM
@@ -431,3 +173,1 @@sqlite3_finalize(stmt);return g_slist_length(*result) != 0 ? DB_RESPONSE_SUCCESS : DB_RESPONSE_EMPTY;return DB_RESPONSE_ERROR;where is logging? it can be hard to catch this place
@@ -448,2 +181,3 @@}return NULL;// Fallback: return an empty message with sane defaultsProfMessage* msg = message_init();nit: no logging again
@@ -0,0 +63,4 @@// bombs) — second occurrence was silently dropped on import. Hashing// whole body is microseconds of extra work per row.GChecksum* sum = g_checksum_new(G_CHECKSUM_SHA256);g_checksum_update(sum, (const guchar*)(timestamp ? timestamp : ""), -1);is it efficient? Might g_strdup_printf first be more efficient than updating checksum each time?
@@ -0,0 +199,4 @@Jid*jid_create(const gchar* const fulljid){if (!fulljid)not a stub
Bug Report: Flatfile Backend Page-Up Scroll Stuck at Top
Summary
The flatfile database backend's page-up (scroll-up) functionality was getting stuck in the middle of the conversation, returning
WIN_SCROLL_REACHED_TOPprematurely and preventing users from loading older messages.Root Cause
The flatfile backend uses a sparse index to efficiently locate messages by byte offset. When paginating up, it uses a cursor-based optimization that stores the byte offset of the oldest message from the previous batch, then reads from the beginning of the file up to that cursor position.
The bug occurred when:
!from_startbackup logic tried to find index entries before this cursor position[X, X)— no bytes were readDB_RESPONSE_EMPTY, which incorrectly set the scroll state toWIN_SCROLL_REACHED_TOPTechnical Details
The cursor-based pagination path (line 753 in
src/database_flatfile.c) was:When
cursor_offsetpointed to the first index entry's byte offset, the subsequent!from_startbackup logic would:entry_idx = 0(the first entry at or afterread_to)margin_entriespositions, butentry_idx - margin_entriesunderflows to 0read_fromto the same offset asread_toFix
Added two additional conditions to the cursor-based pagination check:
The new conditions:
state->n_entries > 0— prevents accessingentries[0]when the index is emptystate->cursor_offset != state->entries[0].byte_offset— prevents cursor-based pagination when the cursor points to the first index entry, forcing fallback to theend_timebisect approachImpact
WIN_SCROLL_REACHED_TOPeven when older messages existedend_timebisect approach, which correctly locates the byte range containing older messagesFiles Modified
src/database_flatfile.c:753— Added safety conditions to cursor-based pagination logic36d260db88to352eb26f71feat(history): flat-file backend with bidirectional SQLite migration
A flat-file alternative to the SQLite chatlog backend with runtime
switching, full migration tooling, integrity verification, and a
synthetic load harness. SQLite remains the default; both backends share
one dispatch layer (db_backend_t vtable) so callers don't change.
Storage layout
flatlog/<account>/<contact>/history.logunder XDG_DATA_HOME, one line per message
(FLATFILE_FORMAT_VERSION); reader warns on missing or mismatched
marker, writer and checker stay in sync via preprocessor
stringification
id,aid,corrects,to,to_res,read) plus escaped body —\|,\],\\,\n,\rliterals prevent log injection
O(log n) time-range lookups; rebuilt on inode / size / mtime
change, extended in-place when the file just grew
stanza_id → from_jid mapping (O(1) MAM dedup, O(1) LMC sender
validation)
Hardening
(
@→_at_, slashes and..rejected at construction); everyper-contact path is anchored under the account's flatlog/
directory and validated before open
ELOOP the operation aborts with an error rather than following
directories with mode 0700; both enforced at creation, verified
on each open and reported on drift by
/history verify0600, random suffix, no name collisions between concurrent
exports), fsync, then rename — partial state never replaces the
live file
every write, including append from live messages and full rewrite
from export, so two profanity processes can't interleave bytes
on the same log
at read with a warning; the parser will not allocate
unbounded memory for a single record
corrects:chain walk stops at thisdepth and emits a warning, preventing a malicious correction
cycle from spinning the apply pass
past 16 leading comment lines, even on garbage input
so a malformed time filter cannot cause an unbounded scan
parse cannot cause a NULL deref on later page-up
not match the original message's sender is rejected at write
time and surfaced via cons_show_error; a cycle in the apply pass
is broken via a visited-set
the literal "(null)" as no resource and returns a bare jid;
similar normalisation for barejid eliminates the legacy
"user@host/(null)" artefact that leaked into stored fulljids
whenever g_strdup_printf("%s", NULL) ran inside create_fulljid
Commands
/history switch sqlite|flatfile— runtime backend swap, closesthe old backend and opens the new one without reconnecting
/history export [<jid>]— SQLite -> flat-file, merging with anyexisting flatlog (dedup keyed on a SHA-256 hash mixing stanza_id,
timestamp, from_jid, body — robust against id reuse by older
clients)
/history import [<jid>]— flat-file -> SQLite, same mergesemantics, runs inside a single SQLite transaction with rollback
on per-contact failure
/history verify [<jid>]— integrity check; emits a structuredlist of issues (ERROR / WARNING / INFO) per file:
BOM present, CRLF line endings, empty file
control characters, unparsable lines, timestamps out of
order, duplicate
id:andaid:(tracked separately so astanza/archive id collision isn't double-reported)
corrects:references whose target id isnot present in the file
/history backend— show currently active backend[sqlite]/[flatfile]in the statusbar next to the JID
flatfile backend is currently active, so migration works
regardless of which backend is live
Tests
key stability, JID normalisation), database_stress (14 cases
exercising rapid writes, large messages, deep LMC chains, MAM
dedup, concurrent contacts)
import round-trip with content equality, MUC migration,
timestamp normalisation across timezones
read, page-up scroll, MAM ingest, mixed workload) and failure
modes F1–F17 (page-up cursor and forward-iteration symmetry,
oversized lines, MAM dedup, LMC depth and cycles, BOM/CRLF,
missing log, empty file, mtime+inode flip, broken corrects, etc.)
emit CSV rows for baseline comparison
b9c3267aadto2990ec795aWIP: no-DB mode implementationto no-DB mode implementation