fix: add lock on config.yaml to prevent TOCTOU coruption#50
fix: add lock on config.yaml to prevent TOCTOU coruption#50Zack (Zackaryia) wants to merge 3 commits into
Conversation
Zack (Zackaryia)
changed the title
Add race_test.go that demonstrates IIP-20714: multiple concurrent read-modify-write cycles on config.yaml without file locking causes invalid YAML, empty file reads, and silent data loss.
Zack (Zackaryia)
force-pushed
the
zshamsi/fix-config-filelock
branch
2 times, most recently
from
7b5b0e5 to
f13abb7
Compare
Concurrent CLI invocations race on config.yaml through the non-atomic read-modify-write in New() (viper.WriteConfig + addDocCommentsToYAML). Wrap the entire critical section in an exclusive flock via gofrs/flock.
Zack (Zackaryia)
force-pushed
the
zshamsi/fix-config-filelock
branch
from
f13abb7 to
378f62c
Compare
There was a problem hiding this comment.
This change is because the file lock fails before writing fails, so we need to edit the error message. Permission denied makes more sense in context.
| if os.Getenv("RACE_WORKER") != "1" { | ||
| t.Skip("skipping: only runs as subprocess of TestRaceConditionEndToEnd") | ||
| } |
There was a problem hiding this comment.
Is there any downside to always enable the running of this test?
There was a problem hiding this comment.
It would fatal from no RACE_DATA_DIR env var.
There was a problem hiding this comment.
Ah, what if we just made a temp directory with t.TempDir()?
I might be misunderstanding what the test is doing, but in principle I feel like we should be able to run this in CI
There was a problem hiding this comment.
Because we need all the processes to use the same config file to test the race condition we'd have to still have RACE_DATA_DIR.
We could do TempDir as a fallback so that when this is individually ran it wont fatal but then the test name wont make sense.
2 participants
Add race_test.go that demonstrates IIP-20714: multiple concurrent read-modify-write cycles on config.yaml without file locking causes invalid YAML, empty file reads, and silent data loss.