file locking: keep the log on a failed roll, fix the mutex name - #319
FreeAndNil wants to merge 3 commits into
Conversation
- A failed rename was reported, then the file reopened without appending, which destroyed it. A backup agent holding a read handle is enough. It appends now, and only that rename is retried, once per MaxFileSize of growth, so the backups are never rotated twice and a retry that succeeds keeps the generation it recovers. - The footer, close and open paths released the file lock even when acquiring it had failed. Only what was taken is released now. audit da18b6f-f036, da18b6f-f032
FreeAndNil
changed the title
- InterProcessLock named its mutex before the path was resolved, so a relative and an absolute spelling of one file took two mutexes and excluded nothing. - A name over 255 characters throws on Unix, out of ActivateOptions, so a deep log path took the appender down. Those are hashed now. Windows has no limit, measured, so its names are left alone and keep excluding older versions. - Both mutexes take their name from one helper, which carries why there is no ACL, no Global\ prefix and no user component. audit da18b6f-f031, da18b6f-f010
Windows 7 SP1 is the floor for the net462 build this file compiles into, so the version test could not fail and the 32766 constant behind it was dead. The surviving constant keeps its measured value and loses the superseded lore.
FreeAndNil
changed the title
FreeAndNil
changed the title
| [SetUp] | ||
| public void SetUp() | ||
| { | ||
| _directory = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N")); |
There was a problem hiding this comment.
very minor: if you'd like to keep stuff in the test, you're welcome to try PeanutButter.Utils' AutoTempFolder - a disposable which creates a folder in the system temp location and destroys it when disposed, and you can use it like:
using var dir = new AutoTempFolder();
var pathToFile = Path.Combine(dir.Path, "some-file");I must admit that I have an aversion to storing state on a test fixture and keeping that state useful via setup/teardown methods - I've found that at worst it introduces shared state that someone forgets is shared, failing a test, and at best, it means that when I'm trying to figure out what a test actually does, I have to hop around the test fixture. Anyhoo, it's just a preference of mine.
| } | ||
| finally | ||
| { | ||
| _stream.ReleaseLock(); |
There was a problem hiding this comment.
there's an expectation that _stream might be null above - but here, there's no guard - suggest _stream?.ReleaseLock()
| } | ||
| finally | ||
| { | ||
| _stream.ReleaseLock(); |
There was a problem hiding this comment.
there is conditional access to _stream above, defending against nulls, but no such defense here - suggest _stream?.ReleaseLock()
| { | ||
| if (locked) | ||
| { | ||
| _stream!.ReleaseLock(); |
There was a problem hiding this comment.
minor: I know that _stream should be non-null, if locked is true - but I'd still recommend using _stream?.ReleaseLock(); - it unburdens the reader from figuring that out.
|
|
||
| if (_rollSize && (File is not null) && ((CountingQuietTextWriter)QuietWriter!).Count >= MaxFileSize) | ||
| if (_rollSize && (File is not null) | ||
| && ((CountingQuietTextWriter)QuietWriter!).Count >= MaxFileSize) |
There was a problem hiding this comment.
minor: we can avoid the casts (and the later pattern-match at 644) by saving our own local reference in SetQWForFiles:
protected override void SetQWForFiles(TextWriter writer)
=> QuietWriter = _countingWriter = new CountingQuietTextWriter(writer, ErrorHandler);
private CountingQuoetTextWriter _countingWriterand then use _countingWriter here, at 576 and 644 - assuming I haven't missed something somewhere, but it looks like the only place this can be set is from the call to SetQWForFiles in FileAppender?
There was a problem hiding this comment.
also: I would rename SetQWForFiles to SetQuietWriterForFiles - again, as the reader, I have to parse what QW is when I read the code & the name change doesn't matter to the compiler.
2 participants
Four findings from an audit, all in file locking. None is a vulnerability;
audit da18b6fd-*cites them for provenance.
now, and only that rename is retried, once per
MaxFileSizeof growth, so the file can exceedMaxFileSizewhile the rename keeps failing.every later acquisition.
of one file excluded nothing.
keeps its names.
The
Local\prefix, per-user scoping and ACL the scan proposed are deliberately absent: measured onWindows, cross-session exclusion never existed, and a bare
Global\prefix throws for whicheverprocess starts second.
Unrelated, also here: a pre-Vista branch in
EventLogAppenderthat could not be false.