From 87afd12fe55c8c02be4f8cef93d5f28ae96c0ecd Mon Sep 17 00:00:00 2001 From: adam Date: Mon, 12 Oct 2026 11:34:41 +0800 Subject: [PATCH] =?UTF-8?q?fix(lru/server/router):=20=E5=90=88=E5=B9=B6?= =?UTF-8?q?=E5=90=8E=E8=AF=84=E5=AE=A1=E7=9A=84=E4=B8=89=E9=A1=B9=E4=BF=AE?= =?UTF-8?q?=E5=A4=8D=20=E2=80=94=E2=80=94=20set=5Fmax=5Ffile=5Fsize=20?= =?UTF-8?q?=E7=A0=96=E5=8C=96=E3=80=81=E8=B7=A8=20server=20=E5=85=B1?= =?UTF-8?q?=E4=BA=AB=E3=80=81=E8=BF=81=E7=A7=BB=E6=B3=A8=E9=87=8A=E5=86=92?= =?UTF-8?q?=E5=85=85=E5=B7=B2=E9=AA=8C=E8=AF=81?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 对 #471/#470/#472/#473 合并后（main d257c92）做批判性精读，修复其中三项。 另两项（set_body 的 OOM 覆盖需要分配失败注入设施、listen_routes 与配置 通道相交）分别单独排期与本 PR 的文档注记。 ## P1：uvhttp_lru_cache_set_max_file_size(0) 砖化缓存 #473 给 lru_cache_set_max_entries / set_max_memory_usage 补了「写点校验」， 漏了同文件的第三个 setter —— 而这个比那两个更毒： - put() 在 lru_cache.c:349 用 `content_length > cache->max_file_size` 对内容 长度做硬截断；create() 又不接收该参数（默认 1GB 写死在 :82）。 - 于是 setter 是该字段唯一守卫，而它裸赋值：写 0 之后**每次 put 都返回 UVHTTP_ERROR_INVALID_PARAM**，缓存永久不可用，调用方只看到一串 "File size exceeds limit" 的 WARN。 - 评审探针实测：set(0) → max_file_size=0 → put 返回 -1。 当时按「create 拒绝的值 setter 不得接受」扫描，漏它的原因正在于此：它不在 create 的参数表里。修法与另两个同范式（告警 + 保持原值），并补 3 个用例 （含核心回归「写入 0 之后 put 仍成功」）。变异验证：撤掉校验 → SetMaxFileSizeRejectsZero / ZeroMaxFileSizeDoesNotBrickTheCache 变红。 ## P1：take_router / take_context 的护栏是每 server 的，关不住跨 server 共享 take 的护栏只查「这一个 server 自己的当前指针」，成功堵住同一 server 收 两次不同对象，但漏了把同一对象交给两个 server： take_router(A, R); take_router(B, R); server_free(A); server_free(B); → 第二次 uvhttp_router_free 是 double free context 同理（server_free 对 context 调 destroy）。ASan 只在 free 时报， 远离赋值点。这是既有行为（旧 set_router 完全无护栏），但本轮把「所有权已 移交、不可替换」写成了头文件契约，而契约比护栏宽 —— 需写明口径。 修法：头文件 @note 明确「一个对象只属于一个 server」。用所有权登记表强制 属 d6 范围（会变成破坏性），不在本 PR。 ## P2：migrate_to_trie 注释把「会被下次尝试复用」当成已验证事实 失败路径留在池中的半成品节点「被复用」这一主张，既无测试也无自然触达路径 （触发条件是 12 子节点上限，而本仓无删路由 API，失败后不可能再成功）。改为 如实描述：池单调增长直到迁移成功或 router 释放；retry 会按段名匹配复用既有 节点 —— 标注「按代码推演，非测试验证」。 ## 另两项不修、只记录 - set_body 的 OOM 原子性无测试牙齿（唯一能区分新旧的是分配失败分支，本仓无 注入设施）——单独排期 - listen_routes 建出的 server 不经 context/config，所有限额走编译期常量 — — 已在头文件 @note 写明，结构性解决属 d5/d6 验证：ctest 120/120（默认构建）；static-files 配置 120/120，其中 LRU 14 个 用例真实执行；cppcheck 零告警；test-validity 门禁 PASS；门禁口径内文件 clang-format 18.1.8 合规。 --- include/uvhttp_server.h | 17 ++++++++++ src/uvhttp_lru_cache.c | 14 ++++++++ src/uvhttp_router.c | 11 +++++-- test/unit/test_lru_cache_capacity.cpp | 46 +++++++++++++++++++++++++++ 4 files changed, 86 insertions(+), 2 deletions(-) diff --git a/include/uvhttp_server.h b/include/uvhttp_server.h index 323ed980..3fe8428e 100644 --- a/include/uvhttp_server.h +++ b/include/uvhttp_server.h @@ -262,6 +262,13 @@ uvhttp_error_t uvhttp_server_stop(uvhttp_server_t* server); * @note Added in v2.10. The five-step sequence remains available for callers * that need to keep the router handle - prefer this for everything else. * + * @note This entry builds a bare server: no `uvhttp_context_t` and no + * `uvhttp_config_t` are attached. Every config-backed setting on the + * result therefore falls back to its compile-time constant (backlog, + * tcp_keepalive_timeout). If you need those tunable, use the five-step + * sequence with a context instead - wiring a config into this path is + * tracked separately. + * * @example * uvhttp_server_t* server = NULL; * uvhttp_route_t routes[] = { @@ -331,6 +338,12 @@ uvhttp_error_t uvhttp_server_set_handler(uvhttp_server_t* server, * uvhttp_server_free. A NULL router is only a no-op when the server * holds none already; once a router has been taken, NULL is rejected * like any other pointer. + * + * @note The guard above is per-server, so it does NOT stop the same router + * from being handed to two different servers. Do that and both + * uvhttp_server_free calls release it: the second one is a double free + * (ASan reports it at free time, far from the assignment). One router + * belongs to exactly one server. */ uvhttp_error_t uvhttp_server_take_router(uvhttp_server_t* server, uvhttp_router_t* router); @@ -348,6 +361,10 @@ uvhttp_error_t uvhttp_server_take_router(uvhttp_server_t* server, * as uvhttp_server_take_router. No detach operation exists: a NULL * context is only a no-op when the server holds none already, and is * rejected once one has been taken. + * + * @note Like take_router, the guard is per-server: handing the same context + * to two servers makes both destroy it - a double free at free time. + * One context belongs to exactly one server. */ uvhttp_error_t uvhttp_server_take_context(uvhttp_server_t* server, struct uvhttp_context* context); diff --git a/src/uvhttp_lru_cache.c b/src/uvhttp_lru_cache.c index 34f0d027..4ffefb66 100644 --- a/src/uvhttp_lru_cache.c +++ b/src/uvhttp_lru_cache.c @@ -677,12 +677,26 @@ void uvhttp_lru_cache_reset_stats(cache_manager_t* cache) { /** * set maximum file size for cache + * + * The write point is the ONLY guard for this field: uvhttp_lru_cache_create + * takes no max_file_size argument (it hardcodes a 1GB default), and put() + * applies it as a hard cutoff (content_length > max_file_size, see the + * "check if file size exceeds limit" branch). A zero written here therefore + * bricks the cache - every later put returns UVHTTP_ERROR_INVALID_PARAM with + * only a WARN as explanation - so it is rejected like the two setters above + * instead of being written silently. */ void uvhttp_lru_cache_set_max_file_size(cache_manager_t* cache, size_t max_file_size) { if (!cache) return; + if (max_file_size == 0) { + UVHTTP_LOG_WARN( + "Ignoring max_file_size=0 (put() would reject every entry)"); + return; + } + cache->max_file_size = max_file_size; } diff --git a/src/uvhttp_router.c b/src/uvhttp_router.c index 374997df..d19837ea 100644 --- a/src/uvhttp_router.c +++ b/src/uvhttp_router.c @@ -352,8 +352,15 @@ static uvhttp_error_t migrate_to_trie(uvhttp_router_t* router) { * already had; only the route that triggered migration fails. * * The trie nodes built so far stay in the pool unreferenced - * (use_trie is 0, so no lookup reaches them) and are reused by - * the next attempt, which walks the array again from the root. + * (use_trie is 0, so no lookup reaches them); they are neither + * freed nor reachable, so the pool grows monotonically until a + * migration succeeds or the router is freed. A retry re-walks + * the array from the root and find_or_create_child matches by + * segment name, so it reuses the nodes already there (traced + * by reading, not by a test - the failure here is the 12-child + * cap, which no route removal can relieve, so the + * "fail then succeed" transition has no test). + * * Freeing or detaching old_routes here would destroy every * previously registered route. */ return UVHTTP_ERROR_OUT_OF_MEMORY; diff --git a/test/unit/test_lru_cache_capacity.cpp b/test/unit/test_lru_cache_capacity.cpp index e3c70a0a..c837edb0 100644 --- a/test/unit/test_lru_cache_capacity.cpp +++ b/test/unit/test_lru_cache_capacity.cpp @@ -196,4 +196,50 @@ TEST_F(LruCacheCapacity, SetMaxEntriesAcceptsPositiveValue) { EXPECT_EQ(cache_->max_entries, 3); } +/* ========== set_max_file_size：写点不变量 ========== */ + +/* + * put() 在「check if file size exceeds limit」分支用 + * if (content_length > cache->max_file_size) → INVALID_PARAM + * 对内容长度做硬截断。而 create() 根本不接收 max_file_size（默认 1GB 写死在 + * lru_cache.c:82），所以 setter 是这个字段唯一的守卫。 + * + * 评审探针实证：set_max_file_size(cache, 0) 之后，每次 put 都以 -1 失败 —— + * 缓存被砖化，调用方只看到一串 "File size exceeds limit" 的 WARN。 + * 本次补上与另两个 setter 同范式的下限校验。 + */ + +TEST_F(LruCacheCapacity, SetMaxFileSizeRejectsZero) { + Create(kBudget); + + uvhttp_lru_cache_set_max_file_size(cache_, 0); + + EXPECT_EQ(cache_->max_file_size, 1024u * 1024 * 1024) + << "create 把默认值写死为 1GB；setter 不得把 0 写进去"; +} + +/* 核心回归：0 不得砖化缓存。砖化的表现是 put 永久 INVALID_PARAM。 */ +TEST_F(LruCacheCapacity, ZeroMaxFileSizeDoesNotBrickTheCache) { + Create(kBudget); + + uvhttp_lru_cache_set_max_file_size(cache_, 0); + + EXPECT_EQ(Put("/after.bin", 64, 's'), UVHTTP_OK) + << "写入 0 之后 put 必须仍能成功（评审探针在此处实测返回 -1）"; + EXPECT_EQ(cache_->entry_count, 1); +} + +/* 合法值仍然立即生效：小于新上限的进得去，大于的被拒 */ +TEST_F(LruCacheCapacity, SetMaxFileSizeAcceptsPositiveValue) { + Create(kBudget); + + uvhttp_lru_cache_set_max_file_size(cache_, 1024); + EXPECT_EQ(cache_->max_file_size, 1024u); + + EXPECT_EQ(Put("/small.bin", 512, 's'), UVHTTP_OK) + << "小于新上限的内容应能放入"; + EXPECT_EQ(Put("/big.bin", 4096, 'b'), UVHTTP_ERROR_INVALID_PARAM) + << "大于新上限的内容应被拒（文件大小判定先于内存预算判定）"; +} + #endif /* UVHTTP_FEATURE_STATIC_FILES */