Skip to content

docs(skill): ban dispatch-level wl_resource type checks in request handlers - #1452

Merged
wineee merged 1 commit into
linuxdeepin:masterfrom
deepin-wm:agent/git-commit/8efed769d2ec
Sep 30, 2026
Merged

wineee merged 1 commit into
linuxdeepin:masterfrom
deepin-wm:agent/git-commit/8efed769d2ec

Conversation

@deepin-wm

@deepin-wm deepin-wm commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

将 WM-558 经验落实进 treeland 协议 SKILL:WM-561

变更内容

.agents/skills/treeland-private-wayland-protocol/SKILL.md(仅文档,净 +6 行):

  1. 新增 Request Handler Argument Validation 一节,收为两段:libwayland 分发层在调用请求处理函数前已完成对象参数校验——非空参数不会为 NULL、未知 id/类型不符/已销毁对象在进入处理函数前即报协议错误、仅 allow-null="true" 参数可为 NULL——因此处理函数无需再校验传入对象是否符合协议声明类型或仍然存活。
  2. 保留分发层管不到的语义层检查:跨 client 归属、inert resource、WSurface::fromHandle / WOutput::fromHandle 判空、数值/枚举合法性、allow-null 参数判 NULL。
  3. Output Requirements 审查清单补充第 5 条:请求处理函数不做分发层已完成的对象校验。

依据:WM-558 / PR #1451。

Summary by Sourcery

Clarify request-handler validation responsibilities in the Treeland Wayland protocol skill.

Enhancements:

  • Document which request-handler argument checks are guaranteed by libwayland dispatch and which semantic checks handlers must still perform.
  • Extend the protocol skill review checklist to prohibit redundant dispatch-layer object validation in request handlers.

Documentation:

  • Add request handler argument validation guidance to the private Treeland Wayland protocol skill.

@deepin-ci-robot

Copy link
Copy Markdown

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@sourcery-ai

sourcery-ai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Updates the Treeland private Wayland protocol skill documentation to clarify that libwayland performs dispatch-level resource validation before request handlers run, while handlers remain responsible for semantic and protocol-specific checks; the review checklist now explicitly enforces this separation.

Sequence diagram for libwayland request validation

sequenceDiagram
    participant Client
    participant Libwayland
    participant Handler

    Client->>Libwayland: request
    Libwayland->>Libwayland: validate object arguments
    alt unknown id, type mismatch, or destroyed resource
        Libwayland-->>Client: protocol error
    else valid object arguments
        Libwayland->>Handler: invoke request handler
        Handler->>Handler: check semantic and protocol-specific conditions
    end
Loading

File-Level Changes

Change Details Files
Define the boundary between libwayland dispatch validation and request-handler semantic validation.
  • Document that dispatch rejects unknown, mismatched, or destroyed resources and guarantees non-nullability for non-optional arguments.
  • Retain handler checks for client ownership, inert or missing wrapper resources, protocol value constraints, and nullable arguments.
.agents/skills/treeland-private-wayland-protocol/SKILL.md
Extend the protocol implementation review checklist to prohibit redundant dispatch-level object checks.
  • Add a checklist item directing reviewers to verify that handlers do not repeat object validation performed by libwayland.
  • Cross-reference the new request-handler validation guidance.
.agents/skills/treeland-private-wayland-protocol/SKILL.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@deepin-wm
deepin-wm force-pushed the agent/git-commit/8efed769d2ec branch 2 times, most recently from 09a9404 to 1c63bea Compare September 30, 2026 02:22
1. libwayland validates object arguments before a request handler runs:
   non-nullable args are never NULL, unknown ids, type mismatches and
   destroyed resources fail dispatch with a protocol error, and only
   allow-null args can arrive as NULL.
2. Condense the Request Handler Argument Validation section in
   treeland-private-wayland-protocol SKILL accordingly: handlers need no
   validation that a passed object matches its protocol-declared type or
   is still alive; the semantic-layer checks stay (cross-client
   ownership, inert resources, missing wrappers, value validity,
   allow-null args).
3. Add the matching Output Requirements checklist item.

Log: codify the WM-558 lesson that libwayland already validates wl_resource arguments, so handlers need no dispatch-level type checks

Influence:
1. Docs-only change, no build or runtime impact.
2. Request-handler code written per the skill should omit dispatch-level type checks.
3. Reviewers can apply the new checklist item when auditing protocol handler patches.

docs(skill): 禁止请求处理函数内分发级 wl_resource 类型检查

1. libwayland 分发层在调用请求处理函数前已完成对象参数校验:非空参数不会
   为 NULL,未知 id、类型不符或已销毁对象在进入处理函数前即报协议错误,
   仅 allow-null 参数可为 NULL。
2. treeland-private-wayland-protocol SKILL 的 Request Handler Argument
   Validation 一节据此收为两段:处理函数无需再校验传入对象是否符合协议
   声明类型或仍然存活,保留语义层检查(跨 client 归属、inert resource、
   缺失包装对象、数值/枚举合法性、allow-null 参数判 NULL)。
3. Output Requirements 审查清单补充对应条目。

Log: 将 WM-558 经验(libwayland 已校验 wl_resource,处理函数无需重复类型检查)落实进协议 SKILL

Influence:
1. 仅文档变更,不影响编译与运行。
2. 后续按该 skill 生成的协议处理代码应省略分发级类型检查。
3. 走查协议处理补丁时可使用新增审查条目。

Multica Issue: WM-561
@deepin-wm
deepin-wm force-pushed the agent/git-commit/8efed769d2ec branch from 1c63bea to 29e6413 Compare September 30, 2026 02:26
@deepin-wm
deepin-wm marked this pull request as ready for review September 30, 2026 02:30

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

@wineee wineee left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

评审结论:LGTM(仅文档变更,技术声明准确)

无 C++ 代码变更(净 +6 行文档),聚焦文档声明的技术正确性。

技术声明逐条核验(对照 libwayland 源码)

新增的 "Request Handler Argument Validation" 一节声明 libwayland 分发层已完成对象参数校验,已在 src/connection.c / src/wayland-server.c 中逐条验证,全部成立:

PR 声明 源码依据 结论
非空参数不会是 NULL wl_connection_demarshal:if (id == 0 && !arg.nullable) → EINVAL ✅
未知 id 报协议错误 wl_closure_lookup_objects:wl_map_lookup 为 NULL 且 id≠0 → "unknown object" ✅
类型不符报协议错误 !wl_interface_equal(object->interface, message->types[i]) → "invalid object ... type" ✅
已销毁对象不再 resolve wl_resource_destroy → wl_map_remove;zombie 检测返回 NULL ✅
仅 allow-null="true" 可为 NULL scanner.c:880 限定 allow-null 仅对 object/string/array 有效 ✅

语义层检查的保留也验证正确(treeland waylib/src/server/kernel/):

  • WSurface::fromHandle / WOutput::fromHandle 确实判空并可能返回 nullptr(if (!handle) return nullptr; ... handle->data)✅
  • "inert resource" 区分尤其有价值:wlr_surface_from_resource 在 wlr 对象已销毁但 client 仍持 wl_resource 时返回 NULL,这是分发层无法感知的(分发层只跟踪 wl_resource 生命周期,不管 wlr 层对象是否 inert)。

建议(均为非阻塞,可改可不改)

  1. 【建议】措辞微调:"Request handlers therefore need no validation that a passed object matches..." → "need not validate whether..." 更通顺。
  2. 【可选】补一个边界限定:"type mismatches fail dispatch" 成立的前提是协议 XML 为该 object 参数声明了具体 interface 类型。libwayland 中 wl_closure_lookup_objects 仅在 message->types[i] != NULL 时才做 wl_interface_equal 检查。treeland 私有协议都声明了类型,不影响结论,但加半句限定可更严谨。
  3. 【可选】cross-client 措辞:服务端 object map 是 per-client 的(client->objects),客户端 A 用客户端 B 的对象 id 会直接命中 "unknown object" 错误;真正需要 wl_resource_get_client 防御的是全局/共享对象(如 wlr_seat_client)归属校验。当前表述不算错,但可更精确。

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deepin-wm, glyvut, wineee

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@wineee
wineee merged commit d3ce9fc into linuxdeepin:master Sep 30, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants