• xpdev: iniSetString misfiles keys into the wrong section when the targ

    From Rob Swindell@1:103/705 to GitLab issue in main/sbbs on Mon Jun 22 01:53:44 2026
    open https://gitlab.synchro.net/main/sbbs/-/issues/1168

    ## Summary

    When an ini **section header exists but the section is empty** (its header is immediately followed by another `[section]` header, or by EOF), `iniSetString()`
    / `iniSetValue()` writes new keys for that section at the **end of the file** —
    under whatever the last section happens to be — instead of into the intended (empty) section. The misfiled keys then cannot be read back via `iniGetString(section, key)`, and repeated rewrites accumulate duplicates.

    ## Root cause

    `section_start()` in `src/xpdev/ini_file.c` (~line 196) returns `strListCount(list)` (the end of the list) when the section is empty:

    ```c
    static size_t section_start(str_list_t list, size_t index)
    {
    char* p = list[index];
    if (p != NULL) {
    SKIP_WHITESPACE(p);
    if (*p == INI_OPEN_SECTION_CHAR) // A new section starts immediately?
    return strListCount(list);
    }
    return index;
    }
    ```

    `find_section()` -> `get_value()` use this as the search/insert start point. For
    an empty section it points past the end of the list, so `ini_set_string()` inserts the new key at the very end of the file (under the last section), and a later `iniGetString()` for the intended section never finds it.

    ## Minimal repro (links libxpdev)

    ```c
    str_list_t b = strListInit();
    iniSetString(&b, "input", "kpturn", "50", NULL);
    iniSetString(&b, "video", "frames_in_flight", "4", NULL);
    iniRemoveKey(&b, "input", "kpturn"); // empties [input]; its header remains
    iniSetString(&b, "input", "mouse", "off", NULL); // intended for [input]
    // iniGetString(b, "input", "mouse", ...) -> NOT FOUND
    ```

    Resulting file:

    ```
    [input]
    [video]
    frames_in_flight=4
    mouse=off <- written under [video], unreadable as input/mouse
    ```

    ## Impact

    Any read-modify-write of an ini file in which a section becomes **empty** (e.g. a
    key removed because it now equals a default) while another section exists below it. Subsequent writes to the emptied section are misfiled and lost on read, and duplicate keys accumulate across rewrites.

    Found while debugging the SyncDOOM door: its per-user prefs file (`data/user/####/doom/syncdoom.ini`) silently stopped saving/loading its `[input]` settings once that section was emptied by "store only non-default keys" logic, and accumulated dozens of duplicate keys under `[video]`.

    ## Proposed fix

    Have `section_start()` return `index` (the position of the next section header, i.e. the start of the empty section's body) instead of `strListCount(list)`:

    ```c
    if (*p == INI_OPEN_SECTION_CHAR) // empty section: insert into its (empty) body
    return index; // was: return strListCount(list);
    ```

    New keys then insert into the empty section correctly, and the read path is unaffected (the `get_value` loop still breaks at the next section header -> "not found"). With this change `section_start()` collapses to always returning `index`, which suggests the empty-section special case may be removable entirely — worth confirming against the ini test suite before committing.

    (The reporting door was fixed independently by writing its prefs file fresh rather than read-modify-write, so this issue is purely about the underlying xpdev behavior.)

    — *Authored by Claude (Claude Code), on behalf of @rswindell*
    --- SBBSecho 3.37-Linux
    * Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)
  • From Rob Swindell@1:103/705 to GitLab note in main/sbbs on Mon Jun 22 18:19:55 2026
    https://gitlab.synchro.net/main/sbbs/-/issues/1168#note_9419

    ## Fixed in `4d5604ba4` (range-29-site)

    Committed to `master` (local; pending push).

    ### What the fix does

    The proposed one-liner was *almost* right but insufficient on its own — applied
    alone it **regressed `iniGetSection()`**: once `section_start()` returns the empty section's body index (the next section's header) instead of the list terminator, `iniGetSection()` — which unconditionally pushed `list[i]` — would
    bleed the following section's header and keys into the result. It only worked before by accident (the old `strListCount` return made `list[i]` NULL).

    So the committed fix is two coordinated parts:

    1. **`find_section()`** now returns the index of the next section header (or the
    list terminator) for an empty section — the correct stop-point for read loops
    *and* the correct insertion point for new keys. The `section_start()` helper
    collapsed to an identity and was removed.
    2. **`iniGetSection()`** now skips its initial push when `list[i]` is a section
    header, so an empty section yields no keys instead of bleeding the next one.

    ### Bonus bug found

    Part 2 also fixes a pre-existing latent bug: `iniGetSection(ROOT)` on a file with
    no root-level keys (one that begins with a `[section]` header) used to return the
    **first named section's** header+keys. That corrupted
    `iniSortSections(list, /*prefix*/ NULL, …)` output — reachable in production from
    `filedat.c`'s `batch_list_sort()` — **duplicating the first entry** of a no-root-keys batch list. Verified on a real file read from disk:

    ```
    iniSortSections(NULL) UNFIXED FIXED
    [first] k1 k2 [first] k1 k2
    [first] k1 k2 <- [second] k1 k2
    [second] k1 k2 (dup gone)
    ```

    Note: no JavaScript `File` ini method reaches this path — `file.iniGetObject()`
    maps to `iniGetNamedStringList()`, which iterates-and-breaks from the section start and was always correct. The bug is only reachable from C callers of `iniGetSection()` / `iniSortSections()`.

    ### Regression test

    Added an in-memory suite under `#ifdef INI_FILE_TEST` (runs when the test binary
    is invoked with no file arguments): 6 checks covering the #1168 misfile, the empty-section read guard, and the empty-root case. It reports **3 failures against the unfixed code and 0 against the fix**, and a fix-1-only build (no `iniGetSection` guard) reports 1 failure — confirming the guard is required, not
    optional.

    — *Authored by Claude (Claude Code), on behalf of @rswindell*
    --- SBBSecho 3.37-Linux
    * Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)
  • From Rob Swindell@1:103/705 to GitLab issue in main/sbbs on Tue Jun 23 15:44:11 2026
    close https://gitlab.synchro.net/main/sbbs/-/issues/1168
    --- SBBSecho 3.37-Linux
    * Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)