fix(http): harden FileCache against short read and stat failure - #880
Merged
Merged
Conversation
- FileCache::Open: on a short read the entry was already put() into the LRU with filebuf.len == st_size, so is_complete() would later report it as complete and serve a partially-filled (garbage-tail) buffer. Remove the entry from the cache on read failure. - file_cache_s::is_modified: stat() result was written directly into st without checking the return value; on failure POSIX leaves the buffer undefined, corrupting st_size/st_mtime (and thus is_complete/etag/ Last-Modified). Stat into a temporary and keep the old st on failure. Co-authored-by: TRAE CLI <traecli@bytedance.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved concurrency and Windows metadata-validation issues block approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request hardens static-file FileCache handling for short reads and failed metadata checks.
Changes:
- Evicts entries after failed reads.
- Preserves cached metadata when POSIX
stat()fails. - Unresolved concurrency and Windows
_wstat()handling issues remain.
File summaries
| File | Summary |
|---|---|
http/server/FileCache.h |
Adds guarded POSIX metadata refresh. |
http/server/FileCache.cpp |
Evicts failed reads, but concurrent publication/removal remains unsafe. |
Review details
Suppressed comments (1)
http/server/FileCache.cpp:111
FileCacheis shared by the server's worker loops, butGet/putandCloseare separate operations. If two requests read the same path concurrently and one fails after another request has inserted a replacement entry, this key-basedClose(filepath)can remove the newer entry rather than the entry whose read failed. Make removal conditional on the cached value still being thisfc(atomically), or serialize the load/remove sequence.
Close(filepath);
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (nread != fc->filebuf.len) { | ||
| hloge("Failed to read file: %s", filepath); | ||
| param->error = ERR_READ_FILE; | ||
| Close(filepath); |
Comment on lines
+37
to
+38
| if (stat(filepath.c_str(), &new_st) != 0) { | ||
| return false; |
Extract stat_and_open() to hide the OS_WIN vs POSIX branching that was duplicated in FileCache::Open, and route is_modified() through the same _wstat/stat split. is_modified() now returns true on stat failure so a removed/failed file forces a reopen (which then reports the error cleanly) instead of serving stale cached content. Co-authored-by: TRAE CLI <traecli@bytedance.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Concurrency and failed-revalidation handling issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
http/server/FileCache.cpp:98
- Removing the entry after
read()fails does not close the concurrent-read window:put()publishes thisfile_cache_tbeforeresize_buf()/read()completes, and another worker can observefilebuf.len == st_sizeviais_complete()and send the uninitialized tail while this thread is still reading.HttpServershares one cache across worker loops, so the entry must be published only after initialization or initialization/reloads must be synchronized with readers; cleanup after failure is not sufficient.
Close(filepath);
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
| if (nread != fc->filebuf.len) { | ||
| hloge("Failed to read file: %s", filepath); | ||
| param->error = ERR_READ_FILE; | ||
| Close(filepath); |
Comment on lines
+38
to
+42
| if (_wstat(hv::utf8_to_wchar(filepath).c_str(), (struct _stat*)&new_st) != 0) | ||
| return true; | ||
| #else | ||
| if (stat(filepath.c_str(), &new_st) != 0) | ||
| return true; |
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.
What
Fixes two correctness bugs in the static-file
FileCache(http/server/FileCache.{h,cpp}).Bug 1: short read poisons the cache
In
FileCache::Open, the entry isput()into the LRU before the file is read, andresize_buf()setsfilebuf.lento the fullst_size. On a shortread()the function reportedERR_READ_FILEand returned NULL, but left a cached entry whosefilebuf.len == st_sizewith an unfilled tail.A later request would hit that entry,
is_complete()(which only comparesfilebuf.len == st_size) would return true, and the server would send the partially-filled buffer — leaking uninitialized memory / serving corrupt content until the mtime changed or the entry was evicted.Fix: remove the entry from the cache (
Close(filepath)) on read failure.Bug 2:
is_modified()corruptsston stat failureis_modified()wrote thestat()result directly intostwithout checking the return value. Whenstat()fails (file removed, permission change, …) POSIX leaves the buffer undefined, corruptingst_size/st_mtime, which then poisonsis_complete(), the ETag and theLast-Modifiedheader.Fix:
stat()into a temporary; on failure keep the previousstand report "not modified" so the cached content is served until it is re-validated or evicted.Testing
make libhv(with--with-http) ✅