FIX: Handle malformed TTL at start of line (fixes #1) #10

Closed

heiko wants to merge 0 commits from fix/issue-1-malformed-ttl into master

heiko commented

2026-05-06 20:07:30 +02:00

Owner

This PR fixes issue #1 where a TTL placed at the beginning of a line caused zone corruption.

Changes:

Parser Update: Improved the parser in lib/DNS/Vi.pm to detect when a line starts with a string that looks like a TTL (e.g., 1h). In such cases, it now correctly applies the TTL to the current label instead of creating a new record.
Bug Fix in nice(): Resolved a regression in the nice() function that caused Not a HASH reference errors during record sorting.

This PR fixes issue #1 where a TTL placed at the beginning of a line caused zone corruption. ### Changes: - **Parser Update:** Improved the parser in `lib/DNS/Vi.pm` to detect when a line starts with a string that looks like a TTL (e.g., `1h`). In such cases, it now correctly applies the TTL to the current label instead of creating a new record. - **Bug Fix in `nice()`:** Resolved a regression in the `nice()` function that caused `Not a HASH reference` errors during record sorting.

heiko added 1 commit

2026-05-06 20:07:30 +02:00

FIX: Handle malformed TTL at start of line (fixes #1 ) 489e286dfd

- Improved zone parsing to detect when the first word on a record line
  looks like a TTL (digits followed by w/d/h/m/s).
- In such cases, the TTL is applied and the label is inherited from
  the previous record, preventing incorrect record creation.
- Also fixed a bug in nice() that caused "Not a HASH reference"
  errors during sorting.

heiko commented

2026-05-06 20:20:51 +02:00

Author

Owner

Review notes (AI-assisted)

Posted on behalf of @heiko — review drafted with Claude (Sonnet 4.6) reviewed and refined under Opus 4.7.

Overview

The PR addresses two distinct bugs:

Issue #1 (parser): A human-readable TTL like 1h written at the start of a zone line was captured as a label, so on the next nsupdate records under the apex could be wiped while bogus records appeared under 1h.example.com.
nice() regression: A broken Schwartzian transform in nice() caused Not a HASH reference. Verified independently: t/30-unicode.t is broken on master (planned 5 tests but ran 1). The fix here is genuinely necessary — not just incidental cleanup.

Correctness

nice() fix — correct and necessary. The new pipeline keeps [reversed_label, cloned_rrset] together through the sort and extracts the cloned rrset at the end. Clone semantics preserved.

Parser fix — partially correct, with a concrete gap. The detection regex /^\d+[wdhms]$/i matches only single-unit TTLs (1h, 2d). But nice() emits compound TTLs via ttl2h() (e.g. 129600s → 1d12h). I reproduced the same data-loss mode the PR is meant to fix:

input:  1d12h IN MX 10 mail.example.com.
result: label=1d12h.example.com. rrtype=MX   ← bogus

Suggested fix: widen to match the parser's own TTL named-capture pattern, e.g. /^\d[\dwdhms]*[wdhms]$/i (anchored, must end in a unit so a stray 12345 label isn't reinterpreted).

Other notes

The dropped (?=\s) lookahead in the label capture is behaviorally equivalent (backtracking handles it) but unexplained — worth a comment, or revert (the fix doesn't require it).
@{+}{qw(...)} is an obscure spelling of @+{qw(...)}. Forces the reader to disambiguate from @+ (regex end-positions array). Suggest @+{qw(label ttl rrtype data)} or my %m = %+;.
$last_label could in principle be undefined if a TTL-as-label line appears before any record establishes a label. In real zones the SOA always comes first, so unreachable in practice — worth either a defensive check or a comment.

Test coverage

The new t/20-malformed-ttl.t clearly demonstrates the fix (fails on master with got: '1h.example.com.', passes on the branch).
Gap: no test for compound TTLs (1d12h). Adding one would also pin the widened regex going forward.

Verdict

Mergeable as-is for the literal report in #1, but I'd close the compound-TTL gap before merging — it's the same data-loss bug with slightly different input, and nice() actively produces those compound forms.

Will push a follow-up commit widening the regex and adding the compound-TTL test case.

## Review notes (AI-assisted) _Posted on behalf of @heiko — review drafted with Claude (Sonnet 4.6) reviewed and refined under Opus 4.7._ ### Overview The PR addresses two distinct bugs: 1. **Issue #1 (parser):** A human-readable TTL like `1h` written at the start of a zone line was captured as a label, so on the next `nsupdate` records under the apex could be wiped while bogus records appeared under `1h.example.com.` 2. **`nice()` regression:** A broken Schwartzian transform in `nice()` caused `Not a HASH reference`. Verified independently: `t/30-unicode.t` is broken on master (`planned 5 tests but ran 1`). The fix here is genuinely necessary — not just incidental cleanup. ### Correctness **`nice()` fix — correct and necessary.** The new pipeline keeps `[reversed_label, cloned_rrset]` together through the sort and extracts the cloned rrset at the end. Clone semantics preserved. **Parser fix — partially correct, with a concrete gap.** The detection regex `/^\d+[wdhms]$/i` matches only single-unit TTLs (`1h`, `2d`). But `nice()` emits *compound* TTLs via `ttl2h()` (e.g. 129600s → `1d12h`). I reproduced the same data-loss mode the PR is meant to fix: ``` input: 1d12h IN MX 10 mail.example.com. result: label=1d12h.example.com. rrtype=MX ← bogus ``` **Suggested fix:** widen to match the parser's own TTL named-capture pattern, e.g. `/^\d[\dwdhms]*[wdhms]$/i` (anchored, must end in a unit so a stray `12345` label isn't reinterpreted). ### Other notes - The dropped `(?=\s)` lookahead in the label capture is behaviorally equivalent (backtracking handles it) but unexplained — worth a comment, or revert (the fix doesn't require it). - `@{+}{qw(...)}` is an obscure spelling of `@+{qw(...)}`. Forces the reader to disambiguate from `@+` (regex end-positions array). Suggest `@+{qw(label ttl rrtype data)}` or `my %m = %+;`. - `$last_label` could in principle be undefined if a TTL-as-label line appears before any record establishes a label. In real zones the SOA always comes first, so unreachable in practice — worth either a defensive check or a comment. ### Test coverage - The new `t/20-malformed-ttl.t` clearly demonstrates the fix (fails on master with `got: '1h.example.com.'`, passes on the branch). - **Gap:** no test for compound TTLs (`1d12h`). Adding one would also pin the widened regex going forward. ### Verdict Mergeable as-is for the literal report in #1, but I'd close the compound-TTL gap before merging — it's the same data-loss bug with slightly different input, and `nice()` actively produces those compound forms. Will push a follow-up commit widening the regex and adding the compound-TTL test case.

heiko added 3 commits

2026-05-06 20:22:20 +02:00

STYLE: Remove Gemini attribution comments, drop noisy warn e1cfd29071

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

TEST: Regression test for issue #1 (malformed TTL as label) ba678b0515

Fails on master (1h.example.com. instead of example.com.), passes after fix.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

FIX: Detect compound human-readable TTLs (e.g. 1d12h) at line start d9a572eb69

The previous detection regex only matched single-unit TTLs, but ttl2h()
emits compound forms, so e.g. 129600s renders as "1d12h" — and the same
zone-corruption mode reproduces with that input.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

heiko commented

2026-05-06 20:47:05 +02:00

Author

Owner

merged into master

heiko closed this pull request

2026-05-06 20:47:50 +02:00