fix a bug in dname scanning

[xofyarg]

A illegal octets was put into Dname, e.g. b"\x03com\x2e", with no check. Composing the name later results malformed message.

Also add a test to convert the zonefile record back and forth to wire format.

[partim]

Thank you for the PR! Do you think it would be worthwhile to add your case to the test zonefiles in test-data?

[xofyarg]

My zone file is as simple as below, and I don't think it will be helpful to cover the issue. I got hit when trying to decode the message on another host. That's why the compose/parse step was added to the test case.

On the other hand, I'd like to ditch the _unchecked and return the error instead. But not sure about the origin design and consequence, it's nice to have your input.

$ORIGIN example.
$TTL 300
@ IN SOA . . 1 10000 2400 604800 1800
  IN NS ns1.example.
  IN NS ns2.example.

[partim]

The unchecked calls are there because the code is supposed to produce a validly encoded domain name and so there can’t be an error.

I don’t yet quite understand what’s going wrong here and, sadly, didn’t have time today to dig into it. Hopefully, tomorrow …

[xofyarg]

I should have written it clearly in the beginning. Let me try to explain. In convert_label, label length prefix is updated whenever the parser reaches a dot, except for when it sees a root label(.)¹. The function will then returns Ok(None), indicating token ends, so no further update to the last dot. Later in scan_dname, it composes a relative dname from buffer without check, meaning the internal content of the relative dname is some binary ended with an ascii dot, e.g. "\x03com."(Display impl of RelativeDname ignores the last dot silently, makes debug more confusing)².

To fix it, scan_dname could either compose a relative dname from buf[..write-1], or a dname from buf[..write] after updating the last dot to \0. I chose the latter as it is a aboslute name in zone file.

Footnotes

1. https://github.com/NLnetLabs/domain/blob/337d0364a46af0f7f7fe8937da6677ce7507d8a5/src/zonefile/inplace.rs#L877 ↩

2. https://github.com/NLnetLabs/domain/blob/337d0364a46af0f7f7fe8937da6677ce7507d8a5/src/zonefile/inplace.rs#L648C38 ↩

[partim]

What seems to happen is that the NS record’s rdlength is off by one – the name itself is correct. I haven’t figured out yet why, but I would rather fix that issue than work around it.

[partim]

Figured it out – the fix is in 9e7907c

I’d still would like to incorporate your compose/parse cycle test, so maybe we should keep both the fix and test in here, which would mean you’d need to adjust your branch. I’m also happy to do all of it in a new PR, whatever way you prefer.

One note for the test: I think it might be good to not exclude the ZoneRecordData::Unknown case – it still checks that the owner name is correctly encoded at least.

[xofyarg]

The PR has been update. I also updated the unknown test case, as it is supposed to have valid rdata.

[partim]

Thank you! Looking good now. I suppose we have enough now to make a release as well.