fix(nvhttp): 修复恢复时app_id不同导致运行应用意外退出#628
Open
ShadowLemoon wants to merge 4 commits intomasterfrom
Open
Conversation
Summary by CodeRabbit发布说明
WalkthroughPR 在 Changes应用启动与恢复流程增强
Sequence DiagramsequenceDiagram
participant Client
participant nvhttp as nvhttp.cpp
participant Proc as proc_t
participant RTSP as RTSPServer
participant Webhook as WebhookSink
Client->>nvhttp: POST /launch 或 /resume (args)
nvhttp->>nvhttp: has_required_*_params(args)
alt resume 且 无活动会话
nvhttp->>nvhttp: use DESKTOP_APP_ID
nvhttp->>Proc: launch_app(appid, args)
else launch 或 resume 有活动会话
nvhttp->>Proc: launch_app(appid, args)
end
Proc-->>nvhttp: launch result + session URL
nvhttp->>RTSP: start RTSP stream for session
nvhttp->>Webhook: emit NV_APP_LAUNCH / NV_APP_RESUME on success
nvhttp-->>Client: XML response (sessionUrl / root.<node> 状态)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/nvhttp.cpp (2)
1815-1934:⚠️ Potential issue | 🟠 Major | ⚡ Quick win把响应节点名参数化，别在复用的
launch_app()里混用resume/gamesession。这个辅助函数现在同时服务
/launch和/resume，但前面的失败分支写的是root.resume，成功分支又固定写成root.gamesession。这样一来，/resume在current_appid == 0走桌面自启动时会返回 launch 形状的 XML；反过来，/launch的部分失败响应也会变成 resume 形状，客户端很容易按错字段判定结果。🩹 建议的最小修复
- void - launch_app(bool &host_audio, resp_https_t response, req_https_t request, const args_t &args, int appid) { + void + launch_app(bool &host_audio, resp_https_t response, req_https_t request, const args_t &args, int appid, const char *result_key) { ... - tree.put("root.resume", 0); + tree.put(std::string("root.") + result_key, 0); ... - tree.put("root.resume", 0); + tree.put(std::string("root.") + result_key, 0); ... - tree.put("root.gamesession", 1); + tree.put(std::string("root.") + result_key, 1); } ... - launch_app(host_audio, response, request, args, appid); + launch_app(host_audio, response, request, args, appid, "gamesession"); ... - launch_app(host_audio, response, request, args, desktop_appid); + launch_app(host_audio, response, request, args, desktop_appid, "resume");As per coding guidelines，
src/**/*.{cpp,c,h}: Sunshine 核心 C++ 源码，自托管游戏串流服务器。审查要点：内存安全、线程安全、RAII 资源管理、安全漏洞。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nvhttp.cpp` around lines 1815 - 1934, The helper launch_app currently mixes two XML shapes by writing "root.resume" in failure branches and "root.gamesession" on success; change launch_app(bool& host_audio, resp_https_t response, req_https_t request, const args_t& args, int appid) to accept a parameter for the response node name (e.g., std::string result_node or an enum), replace all hard-coded "root.resume" and "root.gamesession" uses with tree.put(result_node + ..., including the session presence integer and the status_code/status_message attributes, and update callers for /launch and /resume to pass the appropriate node name so each endpoint produces a consistent XML schema. Ensure status attributes remain at "root.<xmlattr>.status_code" and "root.<xmlattr>.status_message" or parametrize the xml root if intended, and run tests to verify both endpoints return the correct node names.
1815-1857:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift停止通过共享的
host_audio引用保存会话参数。这里存在明确的数据竞争（data race）。
host_audio是start()函数内的局部bool变量，在第 2472-2473 行被两个请求处理器通过引用捕获：https_server.resource["^/launch$"]["GET"] = [&host_audio](auto resp, auto req) { launch(host_audio, resp, req); }; https_server.resource["^/resume$"]["GET"] = [&host_audio](auto resp, auto req) { resume(host_audio, resp, req); };HTTPS 服务器配置了 4 个并发工作线程（第 2548 行
thread_pool_size = 4），这意味着两个处理器可以同时执行。两个处理器都会无保护地写入host_audio：
launch_app()在第 1854-1855 行：host_audio = util::from_view(...)resume()在第 2019-2020 行：host_audio = util::from_view(...)这导致：
- 未同步的读写：无任何互斥锁或同步机制保护访问
- 状态串话：一个客户端的
localAudioPlayMode参数可能被另一个并发请求覆盖，影响该请求的会话配置- 风险加剧：
/resume在无活动应用时会自动拉起 Desktop（第 2009 行调用launch_app），此时更容易复用到旧的或错误的host_audio值应将
host_audio从共享状态改为按请求本地化的参数传递，或使用线程安全的机制（如 thread-local 存储或在请求上下文中存储）。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nvhttp.cpp` around lines 1815 - 1857, The shared mutable bool host_audio causes a data race; stop capturing and passing it by reference. Change launch_app (and resume if applicable) to accept host_audio by value (bool host_audio) and make the request handlers capture per-request value (or construct a local bool from get_arg/localAudioPlayMode inside the lambda) so each request uses its own host_audio; update call sites (https_server.resource lambdas that currently capture [&host_audio]) and follow-through uses like make_launch_session to consume the local/value parameter instead of a shared reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/nvhttp.cpp`:
- Around line 1815-1934: The helper launch_app currently mixes two XML shapes by
writing "root.resume" in failure branches and "root.gamesession" on success;
change launch_app(bool& host_audio, resp_https_t response, req_https_t request,
const args_t& args, int appid) to accept a parameter for the response node name
(e.g., std::string result_node or an enum), replace all hard-coded "root.resume"
and "root.gamesession" uses with tree.put(result_node + ..., including the
session presence integer and the status_code/status_message attributes, and
update callers for /launch and /resume to pass the appropriate node name so each
endpoint produces a consistent XML schema. Ensure status attributes remain at
"root.<xmlattr>.status_code" and "root.<xmlattr>.status_message" or parametrize
the xml root if intended, and run tests to verify both endpoints return the
correct node names.
- Around line 1815-1857: The shared mutable bool host_audio causes a data race;
stop capturing and passing it by reference. Change launch_app (and resume if
applicable) to accept host_audio by value (bool host_audio) and make the request
handlers capture per-request value (or construct a local bool from
get_arg/localAudioPlayMode inside the lambda) so each request uses its own
host_audio; update call sites (https_server.resource lambdas that currently
capture [&host_audio]) and follow-through uses like make_launch_session to
consume the local/value parameter instead of a shared reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b8cf348-c7f6-43a2-9dde-99a55d233d01
📒 Files selected for processing (1)
src/nvhttp.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Windows
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,c,h}
⚙️ CodeRabbit configuration file
src/**/*.{cpp,c,h}: Sunshine 核心 C++ 源码，自托管游戏串流服务器。审查要点：内存安全、 线程安全、RAII 资源管理、安全漏洞。注意预处理宏控制的平台相关代码。
Files:
src/nvhttp.cpp
🪛 Clang (14.0.6)
src/nvhttp.cpp
[error] 9-9: 'algorithm' file not found
(clang-diagnostic-error)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nvhttp.cpp (1)
2070-2083:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWebhook 中重复调用
proc::proc.running()，建议使用已捕获的current_appid第 2000 行已经捕获了
current_appid，此处app_name与app_id又两次调用proc::proc.running()：
- 第一，多了两次锁/原子读取，纯属浪费；
- 第二，存在 TOCTOU 风险：在
launch_session_raise之后到 webhook 构造之间，运行进程理论上可能终止（例如 proc 监控线程检测到子进程退出），导致running()返回 0、get_app_name(0)取到错误名称，与同一事件中其它字段不一致。建议直接使用
current_appid（必要时把get_app_name(current_appid)也提前缓存）：🛡️ 建议使用已捕获的 appid
+ const auto resume_app_name = proc::proc.get_app_name(current_appid); // Send webhook notification for successful resume webhook::send_event_async(webhook::event_t { .type = webhook::event_type_t::NV_APP_RESUME, .alert_type = "nv_app_resume", .timestamp = webhook::get_current_timestamp(), .client_name = launch_session->client_name, .client_ip = net::addr_to_normalized_string(request->remote_endpoint().address()), .server_ip = net::addr_to_normalized_string(request->local_endpoint().address()), - .app_name = proc::proc.get_app_name(proc::proc.running()), - .app_id = proc::proc.running(), + .app_name = resume_app_name, + .app_id = current_appid, .session_id = std::to_string(launch_session->id),As per coding guidelines:
src/**/*.{cpp,c,h}: 审查要点：内存安全、线程安全、RAII 资源管理、安全漏洞。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nvhttp.cpp` around lines 2070 - 2083, The webhook construction calls proc::proc.running() twice which is wasteful and racy; use the previously captured current_appid instead and, if needed, cache the name once via proc::proc.get_app_name(current_appid) before building the webhook payload so app_id and app_name are consistent; update the webhook::event_t fields (app_id and app_name) to use current_appid and the cached app name rather than calling proc::proc.running()/get_app_name() inline.
🧹 Nitpick comments (2)
src/nvhttp.cpp (2)
1795-1813: ⚡ Quick win
find_desktop_app_id的兜底匹配规则过于宽松，可能误匹配到非桌面应用第二个
find_if仅依据app.cmd.empty()进行匹配，而 Sunshine 中任何用户自定义、命令为空的应用都会满足该条件（例如某些仅运行 prep/undo 命令、或纯粹用于切换显示器配置的工具型 App）。当用户没有保留默认 "Desktop" 条目（image_path 非 "desktop" 且 name 非 "Desktop"，例如做了汉化或重命名）时，resume 兜底逻辑可能误启一个非桌面 App，反而触发本 PR 想要修复的"运行中的应用意外退出"语义之外的副作用。建议把兜底匹配限制到 image_path 同时为空（结合 cmd 为空更接近"桌面捕获"语义），或者直接放弃兜底、明确返回 0 让客户端走重新 launch 流程：
♻️ 建议收紧兜底条件
if (desktop == std::end(apps)) { desktop = std::find_if(std::begin(apps), std::end(apps), [](const auto &app) { - return app.cmd.empty(); + return app.cmd.empty() && app.image_path.empty(); }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nvhttp.cpp` around lines 1795 - 1813, The fallback in find_desktop_app_id() is too permissive (it matches any app with app.cmd.empty()), so tighten or remove it: modify the second std::find_if to require both app.cmd.empty() && app.image_path.empty() (so only items that look like a desktop capture match), or eliminate the fallback entirely and return 0 so the client will perform a fresh launch; make the change inside find_desktop_app_id(), keeping the initial lookup by image_path == "desktop" || name == "Desktop" and the final return via util::from_view(desktop->id) unchanged.
1815-1935: 💤 Low value
launch_app抽取良好，但 webhook 构造与 resume 中存在明显重复把原
launch主体抽成launch_app并通过result_node_name区分gamesession/resume两种响应节点，是个干净的统一化重构；fail_guard 中的restore_state()行为也保持了一致。不过
launch_app（1918-1931）和resume（2070-2083）两处构造webhook::event_t的字段几乎一样（仅type/alert_type不同，extra_data的 resolution/fps/host_audio 完全重复）。建议抽个本地辅助函数，在两处调用，便于未来添加新字段时不再两边漏改：♻️ 建议抽取 webhook 构造辅助
static webhook::event_t make_app_event(webhook::event_type_t type, std::string_view alert_type, req_https_t request, const std::shared_ptr<rtsp_stream::launch_session_t> &session, const std::string &app_name, std::int64_t app_id) { return webhook::event_t { .type = type, .alert_type = std::string{alert_type}, .timestamp = webhook::get_current_timestamp(), .client_name = session->client_name, .client_ip = net::addr_to_normalized_string(request->remote_endpoint().address()), .server_ip = net::addr_to_normalized_string(request->local_endpoint().address()), .app_name = app_name, .app_id = app_id, .session_id = std::to_string(session->id), .extra_data = { { "resolution", std::to_string(session->width) + "x" + std::to_string(session->height) }, { "fps", std::to_string(session->fps) }, { "host_audio", session->host_audio ? "true" : "false" } } }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nvhttp.cpp` around lines 1815 - 1935, The webhook::event_t construction is duplicated in launch_app and resume; extract a helper (e.g., static webhook::event_t make_app_event(webhook::event_type_t type, std::string_view alert_type, req_https_t request, const std::shared_ptr<rtsp_stream::launch_session_t>& session, const std::string& app_name, std::int64_t app_id)) that fills timestamp, client/server IPs from request, client_name/session_id from session, app_name/app_id and the extra_data (resolution, fps, host_audio), then replace the inline constructions in launch_app (use launch_session, proc::proc.get_app_name(appid), appid) and in resume (use the resume session and app info) with calls to make_app_event and pass the appropriate type/alert_type before calling webhook::send_event_async.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/nvhttp.cpp`:
- Around line 2070-2083: The webhook construction calls proc::proc.running()
twice which is wasteful and racy; use the previously captured current_appid
instead and, if needed, cache the name once via
proc::proc.get_app_name(current_appid) before building the webhook payload so
app_id and app_name are consistent; update the webhook::event_t fields (app_id
and app_name) to use current_appid and the cached app name rather than calling
proc::proc.running()/get_app_name() inline.
---
Nitpick comments:
In `@src/nvhttp.cpp`:
- Around line 1795-1813: The fallback in find_desktop_app_id() is too permissive
(it matches any app with app.cmd.empty()), so tighten or remove it: modify the
second std::find_if to require both app.cmd.empty() && app.image_path.empty()
(so only items that look like a desktop capture match), or eliminate the
fallback entirely and return 0 so the client will perform a fresh launch; make
the change inside find_desktop_app_id(), keeping the initial lookup by
image_path == "desktop" || name == "Desktop" and the final return via
util::from_view(desktop->id) unchanged.
- Around line 1815-1935: The webhook::event_t construction is duplicated in
launch_app and resume; extract a helper (e.g., static webhook::event_t
make_app_event(webhook::event_type_t type, std::string_view alert_type,
req_https_t request, const std::shared_ptr<rtsp_stream::launch_session_t>&
session, const std::string& app_name, std::int64_t app_id)) that fills
timestamp, client/server IPs from request, client_name/session_id from session,
app_name/app_id and the extra_data (resolution, fps, host_audio), then replace
the inline constructions in launch_app (use launch_session,
proc::proc.get_app_name(appid), appid) and in resume (use the resume session and
app info) with calls to make_app_event and pass the appropriate type/alert_type
before calling webhook::send_event_async.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 386d8b67-b3ff-4be1-823d-0adf4a50b631
📒 Files selected for processing (1)
src/nvhttp.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,c,h}
⚙️ CodeRabbit configuration file
src/**/*.{cpp,c,h}: Sunshine 核心 C++ 源码，自托管游戏串流服务器。审查要点：内存安全、 线程安全、RAII 资源管理、安全漏洞。注意预处理宏控制的平台相关代码。
Files:
src/nvhttp.cpp
🪛 Clang (14.0.6)
src/nvhttp.cpp
[error] 9-9: 'algorithm' file not found
(clang-diagnostic-error)
🔇 Additional comments (2)
src/nvhttp.cpp (2)
2000-2024: 核心修复正确：用current_appid覆盖make_launch_session写入的appid这是本 PR 的关键修复——
make_launch_session内部会把launch_session->appid设置为请求 args 中的appid（往往是 Moonlight 想"切换到"的新应用 ID），与实际正在运行的current_appid不一致时会导致后端把正在运行的应用判为需要终止。在 resume 路径上显式覆盖为current_appid可以正确保持会话连续性。无运行应用时回退到 desktop app 启动并复用
launch_app的设计也比较干净，配合g.disable()避免与launch_app内部的 fail_guard 重复写响应，符合 RAII 用法。
1781-1793: 参数校验辅助函数清晰且与原校验语义一致
has_required_launch_params(args, true)显式要求appid，has_required_resume_params不再要求localAudioPlayMode与appid，与本 PR 中 resume 在缺少 appid 时回退到桌面 App 的语义一致。校验集中后调用点更易读。
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nvhttp.cpp (1)
1796-1841:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift不要在
/launch和/resume之间共享可变的host_audio状态。这里通过
bool &host_audio在请求处理器之间传递状态，而同文件里 HTTPS 服务器配置了 4 个 worker 线程。这个 PR 新增了“无运行应用时/resume直接走 Desktop 启动”的路径，所以缺少localAudioPlayMode的 resume 现在会复用上一个会话、甚至另一个并发请求留下的值；这既有竞态，也会把音频播放模式串到错误的会话里。建议把它改成当前请求 / 当前 session 的局部状态，不要继续依赖进程级共享布尔值。As per coding guidelines，"Sunshine 核心 C++ 源码，自托管游戏串流服务器。审查要点：内存安全、 线程安全、RAII 资源管理、安全漏洞。注意预处理宏控制的平台相关代码。"
Also applies to: 1918-1945, 1981-1985
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nvhttp.cpp` around lines 1796 - 1841, The handler function launch_app currently takes a shared mutable bool& host_audio which causes cross-request races under the multi-threaded HTTPS server; change launch_app to stop using a process-level mutable reference by making host_audio a local bool (or change the parameter to bool host_audio by value) and initialize it per-request from args (via get_arg/localAudioPlayMode) before calling make_launch_session; update any callers (e.g., the /launch and /resume invocations) to pass the per-request value instead of sharing a reference so each session has its own host_audio and no shared state remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/nvhttp.cpp`:
- Around line 1881-1885: proc::proc.execute 返回的 err 不应直接写入 XML 为负值：在处理其返回值（变量
err）时，如果 err == 404 则原样透传到 tree.put("root.<xmlattr>.status_code")，否则把内部错误统一映射为
HTTP 5xx（例如 500 或 503）写入 status_code 并设置合适的 status_message，同时将原始 err 值记录到日志而不是写入
XML；修改涉及的符号有
proc::proc.execute、err、tree.put("root.<xmlattr>.status_code")、tree.put("root.<xmlattr>.status_message")
和 result_node，保持对 result_node 的原有赋值逻辑（tree.put(result_node, 0)）不变。
In `@src/process.cpp`:
- Around line 65-76: The function make_desktop_app constructs a ctx_t but
currently uses uninitialized scalar members (e.g. elevated) by declaring "ctx_t
app;" — change this to value-initialize the struct (e.g. use ctx_t app{} or
equivalent value initialization) so all members start with deterministic
defaults before you assign name, image_path, id, auto_detach, wait_all,
mouse_mode, and exit_timeout in make_desktop_app; this prevents UB from reading
unset fields later.
- Around line 922-933: The retry loop can infinite-loop when the non-indexed
candidate equals the reserved DESKTOP_APP_ID; modify the selection logic in the
loop around calculate_app_id(name, ctx.image_path, i++) so that you treat the
reserved value as a collision: either (A) when std::get<0>(possible_ids) ==
std::to_string(DESKTOP_APP_ID) force selection of std::get<1>(possible_ids) (the
indexed fallback) instead of assigning ctx.id = std::get<0>(possible_ids), or
(B) include the reserved string in the condition that checks ids.count(...) to
reject that candidate immediately; ensure ctx.id is never set to the reserved
DESKTOP_APP_ID and the loop condition (ids.count(ctx.id) != 0 || ctx.id ==
std::to_string(DESKTOP_APP_ID)) can terminate.
---
Outside diff comments:
In `@src/nvhttp.cpp`:
- Around line 1796-1841: The handler function launch_app currently takes a
shared mutable bool& host_audio which causes cross-request races under the
multi-threaded HTTPS server; change launch_app to stop using a process-level
mutable reference by making host_audio a local bool (or change the parameter to
bool host_audio by value) and initialize it per-request from args (via
get_arg/localAudioPlayMode) before calling make_launch_session; update any
callers (e.g., the /launch and /resume invocations) to pass the per-request
value instead of sharing a reference so each session has its own host_audio and
no shared state remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5db7559-a1ab-472b-a03b-d3270642a06a
📒 Files selected for processing (3)
src/nvhttp.cppsrc/process.cppsrc/process.h
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Windows
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,c,h}
⚙️ CodeRabbit configuration file
src/**/*.{cpp,c,h}: Sunshine 核心 C++ 源码，自托管游戏串流服务器。审查要点：内存安全、 线程安全、RAII 资源管理、安全漏洞。注意预处理宏控制的平台相关代码。
Files:
src/process.cppsrc/nvhttp.cppsrc/process.h
🪛 Clang (14.0.6)
src/nvhttp.cpp
[error] 9-9: 'algorithm' file not found
(clang-diagnostic-error)
src/process.h
[error] 11-11: 'limits' file not found
(clang-diagnostic-error)
🪛 Cppcheck (2.20.0)
src/process.cpp
[error] 76-76: Uninitialized variable
(uninitvar)
Comment on lines
+1881
to
+1885
| auto err = proc::proc.execute(appid, launch_session); | ||
| if (err) { | ||
| tree.put("root.<xmlattr>.status_code", err); | ||
| tree.put("root.<xmlattr>.status_message", "Failed to start the specified application"); | ||
| tree.put(result_node, 0); |
There was a problem hiding this comment.
内部执行失败不要直接回填为负的 status_code。
proc::proc.execute() 这里除了 404 还会返回 -1。直接写进 XML 会产生无效状态码，客户端侧的失败分支会变得不可预测。更稳妥的做法是只透传明确的协议状态（比如 404），其余内部错误统一映射到 500/503，原始返回值继续只打日志。
🛠️ 建议修改
- auto err = proc::proc.execute(appid, launch_session);
+ const auto err = proc::proc.execute(appid, launch_session);
if (err) {
- tree.put("root.<xmlattr>.status_code", err);
+ tree.put("root.<xmlattr>.status_code", err == 404 ? 404 : 500);
tree.put("root.<xmlattr>.status_message", "Failed to start the specified application");
tree.put(result_node, 0);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto err = proc::proc.execute(appid, launch_session); | |
| if (err) { | |
| tree.put("root.<xmlattr>.status_code", err); | |
| tree.put("root.<xmlattr>.status_message", "Failed to start the specified application"); | |
| tree.put(result_node, 0); | |
| const auto err = proc::proc.execute(appid, launch_session); | |
| if (err) { | |
| tree.put("root.<xmlattr>.status_code", err == 404 ? 404 : 500); | |
| tree.put("root.<xmlattr>.status_message", "Failed to start the specified application"); | |
| tree.put(result_node, 0); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/nvhttp.cpp` around lines 1881 - 1885, proc::proc.execute 返回的 err 不应直接写入
XML 为负值：在处理其返回值（变量 err）时，如果 err == 404 则原样透传到
tree.put("root.<xmlattr>.status_code")，否则把内部错误统一映射为 HTTP 5xx（例如 500 或 503）写入
status_code 并设置合适的 status_message，同时将原始 err 值记录到日志而不是写入 XML；修改涉及的符号有
proc::proc.execute、err、tree.put("root.<xmlattr>.status_code")、tree.put("root.<xmlattr>.status_message")
和 result_node，保持对 result_node 的原有赋值逻辑（tree.put(result_node, 0)）不变。
Comment on lines
+65
to
+76
| namespace { | ||
| ctx_t | ||
| make_desktop_app() { | ||
| ctx_t app; | ||
| app.name = std::string(DESKTOP_APP_NAME); | ||
| app.image_path = std::string(DESKTOP_APP_IMAGE_PATH); | ||
| app.id = std::to_string(DESKTOP_APP_ID); | ||
| app.auto_detach = true; | ||
| app.wait_all = true; | ||
| app.mouse_mode = 0; | ||
| app.exit_timeout = std::chrono::seconds { 5 }; | ||
| return app; |
There was a problem hiding this comment.
把 ctx_t 显式值初始化，避免 Desktop 路径带出未定义状态。
这里的 ctx_t app; 只填了部分字段，elevated 这类标量成员仍然是未初始化值。当前 Desktop 主要走 placebo 分支，但后面只要有代码读取这些字段就是 UB；直接改成值初始化最稳妥。
🛠️ 建议修改
- ctx_t app;
+ ctx_t app {};
app.name = std::string(DESKTOP_APP_NAME);
app.image_path = std::string(DESKTOP_APP_IMAGE_PATH);
app.id = std::to_string(DESKTOP_APP_ID);
app.auto_detach = true;
app.wait_all = true;
app.mouse_mode = 0;
app.exit_timeout = std::chrono::seconds { 5 };
return app;As per coding guidelines，"Sunshine 核心 C++ 源码，自托管游戏串流服务器。审查要点：内存安全、 线程安全、RAII 资源管理、安全漏洞。注意预处理宏控制的平台相关代码。"
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 76-76: Uninitialized variable
(uninitvar)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/process.cpp` around lines 65 - 76, The function make_desktop_app
constructs a ctx_t but currently uses uninitialized scalar members (e.g.
elevated) by declaring "ctx_t app;" — change this to value-initialize the struct
(e.g. use ctx_t app{} or equivalent value initialization) so all members start
with deterministic defaults before you assign name, image_path, id, auto_detach,
wait_all, mouse_mode, and exit_timeout in make_desktop_app; this prevents UB
from reading unset fields later.
Comment on lines
+922
to
+933
| std::tuple<std::string, std::string> possible_ids; | ||
| do { | ||
| possible_ids = calculate_app_id(name, ctx.image_path, i++); | ||
| if (ids.count(std::get<0>(possible_ids)) == 0) { | ||
| // Avoid using index to generate id if possible | ||
| ctx.id = std::get<0>(possible_ids); | ||
| } | ||
| else { | ||
| // Fallback to include index on collision | ||
| ctx.id = std::get<1>(possible_ids); | ||
| } | ||
| } while (ids.count(ctx.id) != 0 || ctx.id == std::to_string(DESKTOP_APP_ID)); |
There was a problem hiding this comment.
保留 ID 命中时，这个重试循环会卡死。
如果 std::get<0>(possible_ids) 恰好等于 DESKTOP_APP_ID，这里每次都会再次选中这个保留值，因为分支里只把“已存在的 ID”当成冲突，没有把“保留的 Desktop ID”也当成冲突处理。这样 while 条件会一直成立，parse() 会在这条应用记录上无限循环。
🛠️ 建议修改
- std::tuple<std::string, std::string> possible_ids;
+ const auto reserved_id = std::to_string(DESKTOP_APP_ID);
+ std::tuple<std::string, std::string> possible_ids;
do {
possible_ids = calculate_app_id(name, ctx.image_path, i++);
- if (ids.count(std::get<0>(possible_ids)) == 0) {
+ if (std::get<0>(possible_ids) != reserved_id &&
+ ids.count(std::get<0>(possible_ids)) == 0) {
// Avoid using index to generate id if possible
ctx.id = std::get<0>(possible_ids);
}
else {
// Fallback to include index on collision
ctx.id = std::get<1>(possible_ids);
}
- } while (ids.count(ctx.id) != 0 || ctx.id == std::to_string(DESKTOP_APP_ID));
+ } while (ids.count(ctx.id) != 0 || ctx.id == reserved_id);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/process.cpp` around lines 922 - 933, The retry loop can infinite-loop
when the non-indexed candidate equals the reserved DESKTOP_APP_ID; modify the
selection logic in the loop around calculate_app_id(name, ctx.image_path, i++)
so that you treat the reserved value as a collision: either (A) when
std::get<0>(possible_ids) == std::to_string(DESKTOP_APP_ID) force selection of
std::get<1>(possible_ids) (the indexed fallback) instead of assigning ctx.id =
std::get<0>(possible_ids), or (B) include the reserved string in the condition
that checks ids.count(...) to reject that candidate immediately; ensure ctx.id
is never set to the reserved DESKTOP_APP_ID and the loop condition
(ids.count(ctx.id) != 0 || ctx.id == std::to_string(DESKTOP_APP_ID)) can
terminate.
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
1 participant
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.
No description provided.