Skip to content

fix(encode): add depth counter to prevent stack overflow#71

Merged
membphis merged 1 commit into
mainfrom
fix/encode-depth-limit
May 29, 2026
Merged

fix(encode): add depth counter to prevent stack overflow#71
membphis merged 1 commit into
mainfrom
fix/encode-depth-limit

Conversation

@membphis
Copy link
Copy Markdown
Collaborator

@membphis membphis commented May 29, 2026

关联 Issue

Closes #62

做了什么

  • lua/qjson/table.lua 中添加 ENCODE_MAX_DEPTH = 1000 常量(对齐 lua-cjson 默认值)
  • encodeencode_proxyencode_plain_table 添加 depth 参数透传
  • 在四个容器编码函数入口添加深度检查,超限时抛出干净的 Lua 错误:
    • encode_array
    • encode_object
    • encode_lazy_object_walking
    • encode_lazy_array_walking
  • _M.encode = encode 改为包装函数,调用 encode(v, 1) 启动深度计数
  • 新增测试文件 tests/lua/encode_depth_spec.lua,覆盖三种场景

Resolved Checklist Items

  • 在四个编码辅助函数中添加深度计数器(thread-upvalue 或参数传递)
  • 深度超限时抛出 "qjson.encode: max depth exceeded"
  • 默认限制 1000,匹配 lua-cjson
  • 添加 Lua 级别测试:循环引用 + 深度嵌套两种情况

为什么这样做

  • 选择参数传递而非模块级 upvalue:无可变共享状态,天然正确,不需要在错误路径上重置计数器
  • 深度从 1 开始(最外层容器),depth > 1000 触发错误,即 depth=1000 通过、depth=1001 报错——与 lua-cjson 语义一致
  • encode_proxy 的快速路径(dirty=false,直接返回原始 buffer 切片)不做递归,无需检查深度;检查仅在 dirty 走 walking 路径时发生
  • 循环引用无需显式 visited-set:深度限制会在第 1001 次递归时触发干净错误

如何验证

  • make test:131 successes / 0 failures / 0 errors
  • make lint:clippy 无 warning
  • 新增测试覆盖:
    • 循环引用(t.self = t)→ 抛 "qjson.encode: max depth exceeded" 而非 stack overflow
    • 嵌套深度 1001 → 抛错
    • 嵌套深度恰好 1000 → 成功

Breaking Changes

无。_M.encode 公开接口签名不变,depth 仅为内部参数。

Summary by CodeRabbit

  • New Features

    • JSON encoding now enforces a maximum nesting depth limit (1000 levels)
    • Circular table references now raise an error instead of causing a crash
  • Tests

    • Added tests validating maximum depth enforcement and circular reference error handling

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 29, 2026

Warning

Review limit reached

@membphis, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 49 minutes and 23 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a0dd680b-4731-4e48-b277-58e0bf077988

📥 Commits

Reviewing files that changed from the base of the PR and between 07deb85 and 94c2cc5.

📒 Files selected for processing (2)
  • lua/qjson/table.lua
  • tests/lua/encode_depth_spec.lua
📝 Walkthrough

Walkthrough

This PR adds depth-aware JSON encoding to prevent stack overflow from circular references or deeply-nested tables. It introduces ENCODE_MAX_DEPTH (1000), threads a depth parameter through all encode helpers, and seeds depth at public API entry points. Tests verify error handling for circular references and structures exceeding the maximum depth.

Changes

Depth-aware JSON encoding with max-depth limit

Layer / File(s) Summary
Lazy proxy depth-aware encoding
lua/qjson/table.lua
encode_lazy_object_walking and encode_lazy_array_walking accept depth parameter, check against ENCODE_MAX_DEPTH, and pass depth + 1 to recursive calls; encode_proxy forwards depth when dispatching to lazy walkers.
Plain table depth-aware encoding
lua/qjson/table.lua
Main encoder dispatch functions (encode_array, encode_object, encode_plain_table) accept depth, enforce ENCODE_MAX_DEPTH, and propagate depth + 1 through recursive encoding.
Public API and metamethod entry points
lua/qjson/table.lua
Core encode dispatcher changes to encode(v, depth) and is called from _M.encode(v) with depth seeded to 1; LazyObject.__tostring and LazyArray.__tostring call encode_proxy(t, 1) for depth-limited debug stringification.
Depth guard test suite
tests/lua/encode_depth_spec.lua
New test cases verify circular references error cleanly, deeply-nested structures (depth > 1000) raise qjson.encode: max depth exceeded, and structures at exactly depth 1000 encode successfully.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • api7/lua-qjson#60: Both PRs modify lua/qjson/table.lua's lazy-object encoding path (e.g., encode_proxy/encode_lazy_object_walking)—the main PR adds depth-parameter propagation and max-depth checks, while the retrieved PR changes how lazy objects are walked/encoded using cached key order—so they directly overlap in the encoding implementation.
🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
E2e Test Quality Review ⚠️ Warning Tests are unit tests, not E2E tests (only test encode function in isolation). Critical scenario gap: No tests cover lazy table encoding via qjson.decode (a core feature affected by depth checks). Add E2E-style tests that use qjson.decode(json_string) to test depth limits on lazy proxies, including modified lazy tables triggering the walking path; test array depth limits and mixed object/array circular references.
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding a depth counter to the encode function to prevent stack overflow from circular references and deeply nested tables.
Linked Issues check ✅ Passed The pull request addresses all coding requirements from issue #62: depth counter threaded through four encode helpers, max depth limit of 1000 enforced with appropriate error message, and tests for circular references and deep nesting added.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing the depth counter and max depth enforcement specified in issue #62; no out-of-scope modifications detected.
Security Check ✅ Passed No security issues found. PR adds stack overflow protection via depth limiting; no secrets logging, unencrypted storage, auth bypass, or TLS misconfigurations.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/encode-depth-limit

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

…refs and deep nesting

Adds ENCODE_MAX_DEPTH = 1000 (matching lua-cjson default) and threads a
depth argument through all four container encode helpers. Depth is checked
at entry to each container; exceeding the limit raises a clean Lua error
instead of crashing the process via stack overflow.

Fixes #62
@membphis membphis force-pushed the fix/encode-depth-limit branch from 07deb85 to 94c2cc5 Compare May 29, 2026 14:25
@membphis membphis merged commit 2b8f07b into main May 29, 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.

fix(encode): circular reference detection and encode depth limit

1 participant