fix: improve XOrg display handling#1623
Conversation
reddevillg
commented
Mar 20, 2026
reddevillg
commented
- Parse protocol/hostname/display/screen from DISPLAY environment variable
- Add unit tests for display parsing edge cases
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the handling of XOrg display configurations by introducing a more robust and structured parsing mechanism for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the handling of XOrg display configurations by introducing a structured XOrgDisplayConf object to parse the DISPLAY environment variable. The getXOrgDisplay function has been updated to provide a more detailed and robust parsing of display strings, including various protocols, hosts, display numbers, and screen numbers. Comprehensive unit tests have been added to cover a wide range of display string formats and edge cases, which greatly improves the reliability of display parsing. The ContainerCfgBuilder has been adapted to utilize this new structured information for setting up appropriate mounts and environment variables within the container, removing the previous bindXOrgSocket method. Overall, these changes enhance the flexibility and correctness of XOrg display integration.
| } else { | ||
| displayNo = xOrgDisplayConf->displayNo; | ||
| source = fmt::format("/tmp/.X11-unix/X{}", displayNo); | ||
| environment["DISPLAY"] = display; |
There was a problem hiding this comment.
The environment["DISPLAY"] = display; assignment here might lead to inconsistent behavior. If the original display string (e.g., tcp/localhost:0) was parsed by getXOrgDisplay and the else branch is taken, the code proceeds to bind a local Unix socket (/tmp/.X11-unix/X{displayNo}). However, the DISPLAY environment variable inside the container is then set back to the original tcp/localhost:0. This could cause applications within the container to attempt a TCP/IP connection instead of using the bound local Unix socket, potentially breaking XOrg functionality. The DISPLAY environment variable should reflect the actual connection method being used by the container.
environment["DISPLAY"] = fmt::format(":{}", displayNo);
reddevillg
force-pushed
the
fix_xdisplay
branch
from
8f578ab to
4438373
Compare
- Parse protocol/hostname/display/screen from DISPLAY environment variable - Add unit tests for display parsing edge cases Signed-off-by: reddevillg <reddevillg@gmail.com>
reddevillg
force-pushed
the
fix_xdisplay
branch
from
4438373 to
c718363
Compare
deepin pr auto reviewGit Diff 代码审查报告总体评价这段代码主要对显示系统(X11)的配置解析和挂载逻辑进行了重构，将原来简单的路径解析改为更完善的配置解析，增加了对多种显示格式的支持。整体代码质量较好，但有一些可以改进的地方。 具体问题与建议1. 语法与逻辑问题display.cpp 中的字符串处理// 第45行
if (common::strings::starts_with(display, unixPrefix)) {
display = display.substr(unixPrefix.size());
}问题: 建议: 考虑使用 display.cpp 中的异常处理// 第67-73行
try {
result.screenNo = std::stoi(std::string(display.substr(dot + 1)));
if (result.screenNo < 0) {
return LINGLONG_ERR(fmt::format("invalid screen No: {}", result.screenNo));
}
} catch (const std::exception &e) {
return LINGLONG_ERR(fmt::format("failed to get screen No: {}", e.what()));
}问题: 建议: 明确捕获 2. 代码质量问题display.h 中的结构体设计struct XOrgDisplayConf
{
std::optional<std::string> protocol;
std::optional<std::string> host;
int displayNo;
int screenNo;
};问题:
建议: struct XOrgDisplayConf
{
std::optional<std::string> protocol;
std::optional<std::string> host;
int displayNo = 0; // 默认值
int screenNo = 0; // 默认值
// 添加验证方法
bool isValid() const noexcept {
return displayNo >= 0 && screenNo >= 0;
}
};display.cpp 中的重复代码// 第67-73行和第93-103行有类似的异常处理逻辑建议: 提取公共的异常处理逻辑为辅助函数。 3. 代码性能问题display.cpp 中的字符串转换// 第93行
result.displayNo = std::stoi(std::string(display), &s);问题: 每次调用 建议: 考虑使用 int value;
auto [ptr, ec] = std::from_chars(display.data(), display.data() + display.size(), value);
if (ec != std::errc()) {
return LINGLONG_ERR(fmt::format("failed to parse display number"));
}4. 代码安全问题display.cpp 中的路径处理// 第48行
if (std::filesystem::exists(display, ec)) {
result.host = display;
return result;
}问题: 直接使用用户提供的路径，没有进行路径规范化或验证，可能导致路径遍历攻击。 建议: 添加路径验证逻辑： auto normalizedPath = std::filesystem::canonical(display, ec);
if (ec) {
return LINGLONG_ERR(fmt::format("invalid path: {}", ec.message()));
}
// 检查路径是否在允许的目录内
if (!isPathAllowed(normalizedPath)) {
return LINGLONG_ERR(fmt::format("path not allowed: {}", display));
}container_cfg_builder.cpp 中的环境变量处理// 第1028行
if (auto *display = ::getenv("DISPLAY"); display != nullptr) {
auto xOrgDisplayConf = common::display::getXOrgDisplay(display);问题: 直接使用环境变量，没有验证其格式或长度，可能导致缓冲区溢出或其他问题。 建议: 添加环境变量验证： if (auto *display = ::getenv("DISPLAY"); display != nullptr) {
// 验证环境变量长度
if (std::strlen(display) > MAX_DISPLAY_LENGTH) {
LogW("DISPLAY environment variable too long, ignoring");
return false;
}
auto xOrgDisplayConf = common::display::getXOrgDisplay(display);5. 测试覆盖问题display_test.cpp 中的测试用例TEST(DisplayTest, GetXOrgDisplay_IPv6Address)
{
auto result = getXOrgDisplay("[2001:db8::1]:0");
ASSERT_TRUE(result.has_value());
EXPECT_EQ(result->host.value(), "[2001:db8::1]");
// ...
}问题: 测试用例中缺少对错误情况的充分测试，特别是边界条件和异常情况。 建议: 增加以下测试用例：
其他建议
constexpr int ABSTRACT_SOCKET_DISPLAY_NUMBER = 1000;
总结这段代码整体质量较好，重构了显示系统配置解析逻辑，增加了对多种显示格式的支持。主要需要改进的地方包括：
建议在合并前进行更全面的测试，特别是针对边界条件和异常情况的测试。 |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dengbo11, reddevillg The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 participants
