zstream: report invalid record context without assertions

[matthiasgoergens]

Report invalid stream context and oversized BEGIN payloads as ordinary zstream errors, with the record offset, instead of terminating through assertions.

Preserve the existing handling of END records: compound sends have a concluding END outside an individual substream. The non-negative nesting assertion remains an internal invariant.

[Copilot]

🟡 Changes recommended

The payload limit is enforced too late, and regression coverage is missing.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Updates zstream validation to report malformed contexts and oversized DRR_BEGIN payloads as offset-aware errors instead of assertions.

Changes:

• Replaces context assertions with validation errors.
• Preserves compound-stream DRR_END handling.
• Requires early payload-size validation and regression coverage before approval.

File summaries

File	Review summary
cmd/zstream/zstream_validate.c	Critical: enforce the BEGIN payload cap before allocation/read. Nit: add regression tests for invalid contexts, oversized payloads, and offsets.

Review details

Suppressed comments (1)

cmd/zstream/zstream_validate.c:95

• Please add regression coverage for the new validation paths (nested DRR_BEGIN, non-DRR_END records after a substream, and oversized DRR_BEGIN payloads), including the reported offsets. The existing zstream_validate_001_neg test only exercises a malformed DRR_WRITE, so it would not catch these paths regressing to an assertion or disappearing.

		if (context->nesting != 0) {
			errx(1, "nested DRR_BEGIN record at offset %llu",
			    (u_longlong_t)item->dp_stream_offset);
		}

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Retain the nesting invariant assertion to avoid masking internal state corruption.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Retain the non-negative nesting assertion in cmd/zstream/zstream_validate.c.

Review details

Suppressed comments (1)

cmd/zstream/zstream_validate.c:117

• The non-negative nesting invariant is no longer asserted here: removing VERIFY3S(context->nesting, >=, 0) means an internal state violation is silently handled as an external END/context error (or can be accepted as a compound conclusion). The PR description says this invariant should remain internal; retain the assertion before the nesting > 0 branch while still reporting valid zero-nesting END records with errx.

		if (context->nesting > 0) {
			context->nesting--;
		} else if (context->compound &&
		    IS_CONCLUSION(drr, DRR_END)) {
			context->concluded = B_TRUE;

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

No unresolved review comments; validation and regression coverage are included.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[behlendorf]

@GarthSnyder would you mind reviewing this.

[GarthSnyder]

These changes look correct and helpful. Thanks for working on this, @matthiasgoergens.

There's an existing test-stream-creation-scripts directory underneath the ZTS zstream directory which contains Python scripts that generate some of the other test streams. Those scripts aren't run at test time and that directory isn't currently present in a deployed ZTS, but the general pattern is similar. make-invalid-context-streams.py could plausibly either stay where it is ("It's part of the actual test!") or move into that directory ("It's a Python script that generates test streams!").

[Copilot]

🟡 Changes recommended

A critical validation bug would reject normal compound streams.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[matthiasgoergens]

Thanks for the review. I kept make-invalid-context-streams.py alongside the test because this generator runs as part of the test rather than only producing a checked-in fixture. I also tightened the negative cases to require exit status 1, so an assertion abort can no longer satisfy the test.