diff --git a/include/uvhttp_server.h b/include/uvhttp_server.h index 323ed980..3b629520 100644 --- a/include/uvhttp_server.h +++ b/include/uvhttp_server.h @@ -77,13 +77,18 @@ typedef struct { uvhttp_request_handler_t handler; } uvhttp_route_t; -/* Server builder structure (unified API) */ +/* Server builder structure (unified API) + * + * server/router/config are handles the builder exposes for the caller's + * convenience; the builder owns none of them - uvhttp_server_free releases + * all three. The struct is therefore only a view, never an owner, which is + * why a partially built builder is safe to drop with uvhttp_free() alone + * (see create_simple_server_internal). */ typedef struct { uvhttp_server_t* server; uvhttp_router_t* router; uvhttp_config_t* config; uv_loop_t* loop; - int auto_cleanup; } uvhttp_server_builder_t; /* ========== Timeout Statistics Callback ========== */ /** diff --git a/src/uvhttp_server.c b/src/uvhttp_server.c index dcc75215..2343c9fe 100644 --- a/src/uvhttp_server.c +++ b/src/uvhttp_server.c @@ -898,73 +898,78 @@ static uvhttp_error_t create_simple_server_internal( if (!server) { return UVHTTP_ERROR_INVALID_PARAM; } - - uvhttp_server_builder_t* simple = - uvhttp_alloc(sizeof(uvhttp_server_builder_t)); - if (!simple) { - return UVHTTP_ERROR_OUT_OF_MEMORY; - } - - memset(simple, 0, sizeof(uvhttp_server_builder_t)); + /* One out-param invariant, established once here instead of being + * re-asserted in every failure branch: the caller either gets a fully + * listening builder or NULL. */ + *server = NULL; // Loop must be provided by application layer if (!loop) { UVHTTP_LOG_ERROR("loop parameter is required - must be provided by " "application layer"); - uvhttp_free(simple); - *server = NULL; return UVHTTP_ERROR_INVALID_PARAM; } + + uvhttp_server_builder_t* simple = + uvhttp_alloc(sizeof(uvhttp_server_builder_t)); + if (!simple) { + return UVHTTP_ERROR_OUT_OF_MEMORY; + } + memset(simple, 0, sizeof(uvhttp_server_builder_t)); simple->loop = loop; - // Create server - uvhttp_error_t server_result = - uvhttp_server_new(simple->loop, &simple->server); - if (server_result != UVHTTP_OK) { + /* Each sub-object is owned by a local until the single handover point + * below. Every failure between here and there frees its own object by + * name, so no branch ever has to "un-own" anything from the server - + * which is what the previous shape required (it nulled server->config / + * server->router on some branches and not others, and needed a comment + * warning against "fixing" the one branch where nulling would leak). */ + uvhttp_server_t* srv = NULL; + uvhttp_error_t err = uvhttp_server_new(loop, &srv); + if (err != UVHTTP_OK) { uvhttp_free(simple); - *server = NULL; // set to NULL to avoid double release - return server_result; + return err; } - // Create router - uvhttp_error_t router_result = uvhttp_router_new(&simple->router); - if (router_result != UVHTTP_OK) { - // Before calling uvhttp_server_free, set config to NULL - simple->server->config = NULL; - uvhttp_server_free(simple->server); + uvhttp_router_t* router = NULL; + err = uvhttp_router_new(&router); + if (err != UVHTTP_OK) { + uvhttp_server_free(srv); uvhttp_free(simple); - *server = NULL; // set to NULL to avoid double release - return router_result; + return err; } - // Create and set default config - uvhttp_error_t result = uvhttp_config_new(&simple->config); - if (result != UVHTTP_OK) { - // Before calling uvhttp_server_free, set config and router to NULL - simple->server->config = NULL; - simple->server->router = NULL; - uvhttp_router_free(simple->router); - uvhttp_server_free(simple->server); + uvhttp_config_t* config = NULL; + err = uvhttp_config_new(&config); + if (err != UVHTTP_OK) { + uvhttp_router_free(router); + uvhttp_server_free(srv); uvhttp_free(simple); - *server = NULL; // set to NULL to avoid double release - return result; + return err; } - simple->server->config = simple->config; - simple->server->router = simple->router; - simple->auto_cleanup = 1; + /* Ownership handover: exactly one point, reached only on the path that + * built everything. From here on, uvhttp_server_free releases srv, the + * router and the config; the builder struct holds the handles for the + * caller's convenience but owns none of them. */ + simple->server = srv; + simple->router = router; + simple->config = config; + srv->config = config; + srv->router = router; // startlisten - if (uvhttp_server_listen(simple->server, host, port) != UVHTTP_OK) { - UVHTTP_LOG_ERROR("Failed to start server on %s:%d\n", host, port); - // By this point simple->server->config and simple->server->router have - // already been assigned (above), so uvhttp_server_free will release - // them. Do NOT null them here - that would defeat the free and leak - // the router (and its internal allocations). - uvhttp_server_free(simple->server); + err = uvhttp_server_listen(srv, host, port); + if (err != UVHTTP_OK) { + UVHTTP_LOG_ERROR("Failed to start server on %s:%d\n", host ? host : "", + port); + /* Propagate the real reason instead of collapsing every listen + * failure into SERVER_LISTEN: a NULL host is a caller mistake + * (INVALID_PARAM), an occupied port is a runtime one + * (SERVER_LISTEN), and the two deserve different remediation. */ + uvhttp_server_free(srv); uvhttp_free(simple); - *server = NULL; // set to NULL to avoid double release - return UVHTTP_ERROR_SERVER_LISTEN; + return err; } *server = simple; diff --git a/test/unit/test_server_atomic_construct.cpp b/test/unit/test_server_atomic_construct.cpp index 60f2834b..576ff154 100644 --- a/test/unit/test_server_atomic_construct.cpp +++ b/test/unit/test_server_atomic_construct.cpp @@ -415,3 +415,94 @@ TEST(UvhttpServerOwnership, NullContextWithoutContextIsNoOp) { uvhttp_server_free(server); } + +/* ---------- builder 构造路径（create_simple_server_internal） ---------- */ + +/* + * v2.10 d5：builder 构造路径原来在 5 个分支里各自 `*server = NULL`，并在两个 + * 分支里手工置 `server->config/router = NULL`（其中三处其实是无操作——那两项 + * 的所有权要到后面才移交），还靠一条"Do NOT null them here"的注释防止后人 + * "修好"唯一不能置空的分支。 + * + * 现在：out-param 在函数头部一次性建立；子对象由局部变量持有到唯一的移交点； + * 每个失败分支只释放自己命名的对象；移交之后 server_free 一并释放三者。 + * 下面锁住这两条不变式。 + */ + +/* 所有失败路径上 out-param 必须是 NULL（此前未断言过，只是各分支自觉置）。 + * 用哨兵值而非 nullptr 初始化：否则"函数压根没写 out-param"也照样绿。 */ +TEST(UvhttpServerAtomicConstruct, CreateBuilderFailureLeavesOutParamNull) { + uvhttp_server_builder_t* builder = + reinterpret_cast(0x1); + + EXPECT_EQ(uvhttp_server_create(nullptr, "127.0.0.1", 8080, &builder), + UVHTTP_ERROR_INVALID_PARAM); + EXPECT_EQ(builder, nullptr) << "loop 缺失时 out-param 应被写成 NULL"; + + uv_loop_t* loop = uv_default_loop(); + ASSERT_NE(loop, nullptr); + + builder = reinterpret_cast(0x1); + /* host 为 NULL：uvhttp_server_listen 自己拒绝，构造路径据此回滚 */ + EXPECT_EQ(uvhttp_server_create(loop, nullptr, 8080, &builder), + UVHTTP_ERROR_INVALID_PARAM); + EXPECT_EQ(builder, nullptr) << "host 缺失时 out-param 应被写成 NULL"; +} + +/* listen 失败（端口被占）：server 已经接管 router 与 config，清理必须只走 + * server_free —— 这一条正是旧五步法 double free 的同款路径。端口占用用 + * uv_listen 占位实现，与运行身份无关（特权端口在 root 下会绑定成功）。 + * 错误码必须是 SERVER_LISTEN（运行时占用），不是 INVALID_PARAM。 */ +TEST(UvhttpServerAtomicConstruct, CreateBuilderListenFailureLeavesNothing) { + uv_loop_t* loop = uv_default_loop(); + ASSERT_NE(loop, nullptr); + + uv_tcp_t blocker; + ASSERT_EQ(uv_tcp_init(loop, &blocker), 0); + struct sockaddr_in bind_addr; + ASSERT_EQ(uv_ip4_addr("127.0.0.1", 0, &bind_addr), 0); + ASSERT_EQ(uv_tcp_bind(&blocker, (const struct sockaddr*)&bind_addr, 0), 0); + ASSERT_EQ(uv_listen((uv_stream_t*)&blocker, 1, [](uv_stream_t*, int) {}), + 0); + struct sockaddr_in bound_addr; + int addr_len = sizeof(bound_addr); + ASSERT_EQ( + uv_tcp_getsockname(&blocker, (struct sockaddr*)&bound_addr, &addr_len), + 0); + const int occupied_port = ntohs(bound_addr.sin_port); + ASSERT_GT(occupied_port, 0); + + uvhttp_server_builder_t* builder = nullptr; + const uvhttp_error_t err = + uvhttp_server_create(loop, "127.0.0.1", occupied_port, &builder); + + EXPECT_EQ(err, UVHTTP_ERROR_SERVER_LISTEN); + EXPECT_EQ(builder, nullptr) << "listen 失败时不得交出半成品 builder（其 " + "router/config 已归 server 所有）"; + + uv_close((uv_handle_t*)&blocker, nullptr); + uv_run(loop, UV_RUN_NOWAIT); +} + +/* 成功路径的唯一移交点：builder 是视图不是所有者 —— 三个句柄必须指向 + * server 持有的同一批对象（移交与"server 侧接管"是同一个动作）。 */ +TEST(UvhttpServerAtomicConstruct, BuilderIsAViewNotAnOwner) { + uv_loop_t* loop = uv_default_loop(); + ASSERT_NE(loop, nullptr); + + uvhttp_server_builder_t* builder = nullptr; + ASSERT_EQ(uvhttp_server_create(loop, "127.0.0.1", 0, &builder), UVHTTP_OK); + ASSERT_NE(builder, nullptr); + + EXPECT_EQ(builder->server->config, builder->config) + << "config 必须在同一个移交点交给 server"; + EXPECT_EQ(builder->server->router, builder->router) + << "router 必须在同一个移交点交给 server"; + EXPECT_NE(builder->server->config, nullptr); + EXPECT_NE(builder->server->router, nullptr); + EXPECT_EQ(builder->loop, loop); + + /* simple_free 只释放 server（连带 router/config）与 builder 自身； + * 若 builder 也拥有这些对象，这里就是 double free（ASan 会报）。 */ + uvhttp_server_simple_free(builder); +}