Repository navigation
console_edit: save monitor history without touching the process umask - #722
Open
jamesbraid wants to merge 1 commit into
Open
jamesbraid wants to merge 1 commit into
jamesbraid wants to merge 1 commit into
Conversation
rustyline's save_history sets the umask to 0177 around creating the history file. The umask is per process, not per thread, so any other thread that creates a file or directory in that window gets it with no permission bits. Under cargo test the history_is_capped_and_survives_a_restart test saves once per line, 306 times, and the other tests in the process share the umask while it runs. They failed with PermissionDenied on directories created without the execute bit (mips_dis symbol tables, trace files) and the CHD tests failed with InvalidFile when libchdman could not create or open its files. A running monitor has the same exposure: a save races with any file the machine opens on another thread. Create the history file ourselves, owner-only, before handing it to append_history, which then only appends to or rewrites an existing file and never reaches the umask call. A new test pins the owner-only mode. Measured with the full lib on a 224-thread host, 4 runs each. Failed block_node tests per run before: 3, 7, 3, 2; after: 0, 1, 0, 0. The one left is cow_refuses_an_overlay_another_process_holds, a different failure, not addressed here: its reopen after dropping the lock still saw it held. The mips_dis and cpu::trace tests failed in 2 runs each before and in none after. Filtering to the block_node, mips_dis, console_edit and cpu::trace tests, 20 runs: before 20/20 failed, after 0/20. The full lib still fails on tests unrelated to this change, such as missing vpro goldens and the physical decode table limit when 224 tests run at once.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
block_nodeCHD tests, themips_dissymbol-table tests andcpu::tracefail intermittently incargo test --lib. One test is the cause:history_is_capped_and_survives_a_restartcalls rustyline'ssave_historyonce per line, and that call sets the whole process's umask to 0177 around creating the file. Other tests creating files or directories in that window get them without permission bits.The fix creates the history file owner-only itself and uses
append_history, which does not touch the umask for an existing file. This also removes the same window from a running monitor.Measured on a 224-thread host: the four module filters over 20 runs failed 20/20 before and 0/20 after. In four full-lib runs, failed
block_nodetests went 3, 7, 3, 2 before and 0, 1, 0, 0 after. The remaining one,cow_refuses_an_overlay_another_process_holds, is a different failure and not addressed here. The full lib still has other failures unrelated to this change (missing vpro goldens, the decode table limit when 224 tests run at once).