Skip to content

Commit bc30edc

Browse files
committed
fix: address race conditions and safety issues in FileCacheEx
- Add per-entry mutex to file_cache_ex_s for thread-safe mutations - Hold fc->mutex during is_modified() / stat check to prevent data race - Hold fc->mutex during resize_buf() / read to prevent use-after-free - Make prepend_header() thread-safe with lock_guard - Defer put() into LRU cache until entry is fully initialized (prevents stale/incomplete cache entries on ERR_OVER_LIMIT) - Use read() loop to handle partial reads (EINTR) - Invalidate httpbuf pointers after resize_buf() reallocation - Thread-safe get_header_used() / get_header_remaining() accessors
1 parent a90085a commit bc30edc

2 files changed

Lines changed: 36 additions & 10 deletions

File tree

http/server/FileCacheEx.cpp

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ file_cache_ex_ptr FileCacheEx::Open(const char* filepath, OpenParam* param) {
2929
#endif
3030
bool modified = false;
3131
if (fc) {
32+
std::lock_guard<std::mutex> lock(fc->mutex);
3233
time_t now = time(NULL);
3334
if (now - fc->stat_time > stat_interval) {
3435
fc->stat_time = now;
@@ -89,26 +90,36 @@ file_cache_ex_ptr FileCacheEx::Open(const char* filepath, OpenParam* param) {
8990
time(&fc->open_time);
9091
fc->stat_time = fc->open_time;
9192
fc->stat_cnt = 1;
92-
put(filepath, fc);
93+
// NOTE: do NOT put() into cache yet — defer until fully initialized
9394
} else {
9495
param->error = ERR_MISMATCH;
9596
return NULL;
9697
}
9798
}
99+
// Hold fc->mutex for the remainder of initialization
100+
std::lock_guard<std::mutex> lock(fc->mutex);
98101
if (S_ISREG(fc->st.st_mode)) {
99102
param->filesize = fc->st.st_size;
100103
// FILE
101104
if (param->need_read) {
102105
if (fc->st.st_size > param->max_read) {
103106
param->error = ERR_OVER_LIMIT;
107+
// Don't cache incomplete entries
104108
return NULL;
105109
}
106110
fc->resize_buf(fc->st.st_size, max_header_length);
107-
int nread = read(fd, fc->filebuf.base, fc->filebuf.len);
108-
if (nread != (int)fc->filebuf.len) {
109-
hloge("Failed to read file: %s", filepath);
110-
param->error = ERR_READ_FILE;
111-
return NULL;
111+
// Loop to handle partial reads (EINTR, etc.)
112+
char* dst = fc->filebuf.base;
113+
size_t remaining = fc->filebuf.len;
114+
while (remaining > 0) {
115+
int nread = read(fd, dst, remaining);
116+
if (nread <= 0) {
117+
hloge("Failed to read file: %s", filepath);
118+
param->error = ERR_READ_FILE;
119+
return NULL;
120+
}
121+
dst += nread;
122+
remaining -= nread;
112123
}
113124
}
114125
const char* suffix = strrchr(filepath, '.');
@@ -133,6 +144,8 @@ file_cache_ex_ptr FileCacheEx::Open(const char* filepath, OpenParam* param) {
133144
gmtime_fmt(fc->st.st_mtime, fc->last_modified);
134145
snprintf(fc->etag, sizeof(fc->etag), ETAG_FMT,
135146
(size_t)fc->st.st_mtime, (size_t)fc->st.st_size);
147+
// Cache the fully initialized entry
148+
put(filepath, fc);
136149
}
137150
return fc;
138151
}

http/server/FileCacheEx.h

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,16 @@
2525
#include "hstring.h"
2626
#include "LRUCache.h"
2727

28+
// Forward declare to avoid header pollution
29+
struct file_cache_ex_s;
30+
2831
// Default values — may be overridden at runtime via FileCacheEx setters
2932
#define FILECACHE_EX_DEFAULT_HEADER_LENGTH 4096 // 4K
3033
#define FILECACHE_EX_DEFAULT_MAX_NUM 100
3134
#define FILECACHE_EX_DEFAULT_MAX_FILE_SIZE (1 << 22) // 4M
3235

3336
typedef struct file_cache_ex_s {
37+
mutable std::mutex mutex; // protects all mutable state below
3438
std::string filepath;
3539
struct stat st;
3640
time_t open_time;
@@ -56,30 +60,39 @@ typedef struct file_cache_ex_s {
5660
}
5761

5862
// Fixed: avoids shadowing struct stat member with stat() call
63+
// NOTE: caller must hold mutex
5964
bool is_modified() {
6065
time_t mtime = st.st_mtime;
6166
::stat(filepath.c_str(), &st);
6267
return mtime != st.st_mtime;
6368
}
6469

70+
// NOTE: caller must hold mutex
6571
bool is_complete() {
6672
if (S_ISDIR(st.st_mode)) return filebuf.len > 0;
6773
return filebuf.len == (size_t)st.st_size;
6874
}
6975

76+
// NOTE: caller must hold mutex — invalidates filebuf/httpbuf pointers
7077
void resize_buf(int filesize, int reserved) {
7178
header_reserve = reserved;
7279
buf.resize(reserved + filesize);
7380
filebuf.base = buf.base + reserved;
7481
filebuf.len = filesize;
82+
// Invalidate httpbuf since buffer may have been reallocated
83+
httpbuf.base = NULL;
84+
httpbuf.len = 0;
85+
header_used = 0;
7586
}
7687

7788
void resize_buf(int filesize) {
7889
resize_buf(filesize, header_reserve);
7990
}
8091

81-
// Returns true on success, false if header exceeds reserved space
92+
// Thread-safe: prepend header into reserved space.
93+
// Returns true on success, false if header exceeds reserved space.
8294
bool prepend_header(const char* header, int len) {
95+
std::lock_guard<std::mutex> lock(mutex);
8396
if (len > header_reserve) return false;
8497
httpbuf.base = filebuf.base - len;
8598
httpbuf.len = len + filebuf.len;
@@ -88,10 +101,10 @@ typedef struct file_cache_ex_s {
88101
return true;
89102
}
90103

91-
// --- accessors ---
104+
// --- thread-safe accessors ---
92105
int get_header_reserve() const { return header_reserve; }
93-
int get_header_used() const { return header_used; }
94-
int get_header_remaining() const { return header_reserve - header_used; }
106+
int get_header_used() const { std::lock_guard<std::mutex> lock(mutex); return header_used; }
107+
int get_header_remaining() const { std::lock_guard<std::mutex> lock(mutex); return header_reserve - header_used; }
95108
bool header_fits(int len) const { return len <= header_reserve; }
96109
} file_cache_ex_t;
97110

0 commit comments

Comments
 (0)