Skip to content

fix(diagram): set an empty PlantUML operand id when there is none - #2827

Open
nefayran wants to merge 1 commit into
DeusData:mainfrom
nefayran:fix/2818-puml-edge-operand
Open

nefayran wants to merge 1 commit into
DeusData:mainfrom
nefayran:fix/2818-puml-edge-operand

Conversation

@nefayran

Copy link
Copy Markdown

Fixes #2818

What does this PR do?

dg_puml_operand set *label at the start but *id only on its success paths. When an opener has no closer, as in a message to or from the diagram edge (Alice ->: x, Alice <-: x, [-> Alice: x) or a declaration such as class [Unclosed, it returned with *id unwritten. dg_puml_relation and dg_puml_decl then passed an uninitialized dg_str_t to dg_element, which crashed the index worker on some stacks.

The fix sets *id to an empty string at the start of dg_puml_operand, next to *label. The relation callers then take their existing !lid.n / !rid.n exits, so an edge message is skipped, and dg_element returns NULL for a declaration with no id. Other PlantUML lines are not affected.

diagrams_plantuml_edge_messages in tests/test_diagrams.c extracts a file with the three edge forms, the unclosed declaration and one normal message, and checks that it gives the two participants and the one Alice -> Bob arrow.

How I checked it, on macOS with Apple clang:

  • I linked the unchanged internal/cbm/diagram.c into a small driver that runs the PlantUML extractor on one source, built three ways. With -ftrivial-auto-var-init=pattern, Alice ->: x, Alice <-: x, [-> Alice: x and class [Unclosed each segfault. A plain -O1 build segfaults on [-> Alice: x. Under ASan the same line reports a global-buffer-overflow read in dg_element, called from dg_puml_relation, and Alice ->: x produces an arrow to a garbage element.
  • With the fix, every case exits cleanly in all three builds and gives the expected elements and arrows.
  • diagram.c and tests/test_diagrams.c compile with the project's -Wall -Wextra -Werror flags.

I have not run scripts/test.sh locally yet, so CI is the first full run of the suite.

Written with Claude Code (Claude Opus 5.5).

Checklist

  • Every commit is signed off (git commit -s)
  • Tests pass locally (make -f Makefile.cbm test)
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior is covered by a test (reproduce-first for bug fixes)

dg_puml_operand set *label up front but *id only on its success paths, so an
opener without its closer (`Alice ->: x`, `[-> Alice: x`, `class [Unclosed`)
returned with *id unwritten. dg_puml_relation and dg_puml_decl then read an
uninitialized dg_str_t and passed it to dg_element, which crashed the index
worker on some stacks. The operand now starts with an empty id, so those lines
take the existing `!lid.n` / `!rid.n` exits and declarations make no element.

Fixes DeusData#2818

Signed-off-by: Rodion Kazennov <nefayran@gmail.com>
@nefayran
nefayran requested a review from DeusData as a code owner October 29, 2026 05:02
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@pcristin

Copy link
Copy Markdown
Contributor

Written with Codex on behalf of @pcristin.

I’m closing our duplicate #2829 in favor of this earlier PR. The independent results below are for our signed-off commit f61fcf69, whose production initializer matches this PR; the regression tests differ.

Additional coverage is available in diagrams_plantuml_unterminated_operands, plus its registration. Its 16 cases cover unmatched brackets, parentheses, colons and quotes on relation endpoints, declarations and aliases, checking that the following valid message survives without extra elements. The edge-message test also checks include-token retention for each reported edge form.

On Linux x86_64:

  • Both new tests reproduced the pre-fix ASan crash with GCC 13 and stack pattern initialization. After the fix, all 20 diagram tests pass under Clang 20.1.2 ASan/UBSan with stack pattern initialization, without sanitizer warnings.
  • The complete canonical test entry exited 0: 10,025 passed, 0 failed, 10 Windows-only skips across all 191 suites, including all 164 incremental tests and the enabled production lifecycle, worker, watcher, TLS and scope guards.
  • Full CI-mode lint passed with pinned cppcheck 2.20.0 and clang-format 20, and clang-tidy on the exact committed diff passed—the contribution gate implemented in fix(hooks): scope pre-commit clang-tidy to staged changes (#1264) #1310. Full-file diagram analysis removes the uninitialized-branch diagnostic; all 86 remaining diagnostics match the parent’s 87.
  • The canonical make -f Makefile.cbm security target passed: robustness 32/32, vendored integrity 1,083 files.
  • Native indexing of unmodified C4-PlantUML f998c468 indexed all 103 tracked .puml files: 118 diagrams, 758 elements, 413 arrow edges. There was one partial Markdown parse in LayoutOptions.md and three ignored GIF assets. This validates static indexing, without claiming remote-include expansion or rendering.

Full local command:

CBM_TEST_PAR_JOBS=2 CBM_TAIL_JOBS=1 scripts/test.sh \
  BUILD_DIR=build/issue-2818-full CC=clang-20 CXX=clang++-20 \
  'CXX_STDLIB=-lstdc++ -fsanitize-link-c++-runtime'

Limits: broader logs contain UBSan warnings in unchanged Perl, signature and RST paths also reported in #2814; the whole suite is not claimed sanitizer-clean. A whole-tree manual tidy sweep was stopped at 40/242 files after confirming existing findings. An initial entry hit an unchanged selector-fixture SIGPIPE; the unchanged rerun passed.

Our Linux CI full leg, TSan, CodeQL, lint and static security also passed. Overall CI was not green: the UI job failed during onnxruntime-node installation with a network timeout, before UI tests. GitHub denied the single-job retry because repository-admin permission is required.

This branch has not been deployed

No deployments
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.

Index worker segfaults on PlantUML edge messages: dg_puml_operand leaves *id uninitialized

2 participants