Skip to content

Lock config/cache files during read-modify-write - #21

Open
elhoim wants to merge 1 commit into
MISP:mainfrom
elhoim:fix/18-config-file-locking
Open

Lock config/cache files during read-modify-write#21
elhoim wants to merge 1 commit into
MISP:mainfrom
elhoim:fix/18-config-file-locking

Conversation

@elhoim

@elhoim elhoim commented Aug 31, 2026

Copy link
Copy Markdown
Member

Finding 18 (Medium) — bin/cli.py:706

Problem

load_config/save_config and load_cache/save_cache have no locking around the read-modify-write. Two concurrent --configure-module runs each load, mutate and overwrite the whole file; the last writer silently discards the other's credential.

Fix

Finding #18 (config-file-locking, medium): load_config/save_config and load_cache/save_cache had no locking around their read-modify-write cycle, so two concurrent --configure-module runs (or two concurrent queries populating the cache) could each load, mutate and overwrite the whole file — the last writer silently discarding the other's saved credential/cache entry. Added a locked_file() context manager (bin/cli.py) that takes an exclusive flock on a sibling <path>.lock file, and used it to serialize (1) the configure_module load-mutate-save sequence around config_path, and (2) the cache save at the end of main, which now re-reads the cache file under the lock and merges in this run's new entries before writing, so a concurrent run's cache writes made in the meantime aren't clobbered.

Verification

Reproduced against the unmodified code at 9b8c605, then re-checked after the change.

Before
Two subprocess CLI-style workers (worker2.py) both call load_config -> mutate -> save_config on the same /tmp/race_config.json, worker A sleeping 1.0s between load and save, worker B sleeping only 0.1s and starting 0.3s later:

[moduleA] loaded at 0.000: {'modules': {}}
[moduleB] loaded at 0.000: {'modules': {}}
[moduleB] saved at 0.109: {'modules': {'moduleB': {'apikey': 'secretB'}}}
[moduleA] saved at 1.017: {'modules': {'moduleA': {'apikey': 'secretA'}}}

Final /tmp/race_config.json:
{
  "modules": { "moduleA": { "apikey": "secretA" } }
}

moduleB's saved credential is silently lost — worker A's stale in-memory copy (loaded before B's write) overwrote the whole file.
After
Same two workers, now acquiring locked_file(CONFIG) around the whole load-mutate-save sequence:

[moduleB] loaded at 0.027: {'modules': {}}
[moduleB] saved at 0.131: {'modules': {'moduleB': {'apikey': 'secretB'}}}
[moduleA] loaded at 0.000: {'modules': {'moduleB': {'apikey': 'secretB'}}}   (blocked on lock until B released it, then re-read)
[moduleA] saved at 1.017: {'modules': {'moduleB': {'apikey': 'secretB'}, 'moduleA': {'apikey': 'secretA'}}}

Final /tmp/race_config.json:
{
  "modules": {
    "moduleA": { "apikey": "secretA" },
    "moduleB": { "apikey": "secretB" }
  }
}

Both credentials survive. Also verified: `python bin/cli.py --help` exits 0, and the module imports cleanly via importlib (locked_file present on the module).

python bin/cli.py --help exits 0 and the module still imports cleanly. Verification was performed offline against the pure functions — no running misp-modules instance is required.

Branched from 9b8c605. This PR addresses only this finding; the other findings from the same review are in separate PRs, so they will need rebasing against each other as they merge.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DYX4TKA5inzByJ4qGWKjqh

load_config/save_config and load_cache/save_cache had no locking around
the read-modify-write cycle. Two concurrent --configure-module runs (or
two concurrent queries that both populate the cache) would each load,
mutate, and overwrite the whole file, so the last writer silently
discarded the other's update (e.g. a saved module credential).

Add a locked_file() context manager that takes an exclusive flock on a
sibling .lock file, and use it to serialize the configure_module
load-mutate-save sequence and the cache save (re-reading and merging
entries under the lock so a concurrent run's cache writes aren't
clobbered).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant