Return error for empty target

[taoso]

try to fix #1626

[miekg]

thanks this looks a lot better

[miekg]

technically dns is 8-bit clean, and \n is sort of a valid character in a dns name, so this corner case must be dealt with in the zone parser and not here

[taoso]

@miekg after some dig into the parser code, it seems impossible to deal this case in the zone parser.

Because all the rr depends on the toAbsoluteName() need to deal this case. But different rr has different rdata format.

For example,

• the CNAME has the rdata format of example.org.
• the MX has the rdata format of 50 mail
• the SVR has the rdata format of 0 5 5060 sipserver.example.com.
• the HTTPS has the rdata format of HTTPS 1 . port=8002

You cannot detect the bad target without knowing the rdata format.

Fortunately, all target in the rdata need to use the toAbsoluteName() method to uniform the domain. It's seems appropriate to add the check logic in the toAbsoluteName() method.

[miekg]

Indeed, but it may be possible to detect "no data" seen when a comment token appears? It's not the newline you care about but the fact nothing is specified until you see a comment token.

Don't know if that's easy to do or not, but somewhere in that area you have to look for a solution

[taoso]

the fact nothing is specified until you see a comment token.

This is only true for the CNAME case. For the rr with rdate containing both target and other information like MX or SVR, it fails.

For example, in bad.example.org. MX 10 ; bad mx, the target is after the priority of 10. If we want to deal this case, it seems we need to add some logic in the case zExpectRdata: branch. However, in the MX example, its rdata is not empty, but still invalid. The parser main loop cannot detect the invalid target because it does not know the rdata format.

If checking target is not accepted in the toAbsoluteName() method, it seems no choose by do the validation in the parse() method of every related rr separately. There are more that 30 different rr types need to be updated.

[miekg]

toAbsolute is def. the wrong place, I also see you point about the number of RR types.

[taoso]

If the toAbsoluteName is the wrong place, would you kindly like to make any suggestion on which part could the check logic be placed? Or each RR that requires domain data should check in their own parse() method?

[miekg]

I think the latter, unless something more general could be find in the parser itself - but I doubt it, because it ( think it only lexes) doesn't have this information. Maybe something more generic can be found about the RRs having this problem, i.e. why does A not run into this if CNAME does

[miekg]

	_, err := NewRR("miek.nl. IN CNAME	")
	t.Error(err)
	_, err = NewRR("miek.nl. IN A	")
	t.Error(err)

gives:

=== RUN   TestZoneParseWrongComment
    scan_test.go:473: <nil>
    scan_test.go:475: dns: bad A A: "\n" at line: 1:14

and that error is from scan_rr.go. Guess we can look at whitespace just there, because this is def. not something that you need or want.

And looking further that toAbsolute function might be the best place, this in scan.go and not exported, however '\n' is not enough... you most def do not want TrimSpace or anything else here in the hot path either

[miekg]

hm,, it looks like everything is indeed eaten except the newline (this makes sense as whitespace in this section of the RR should be insignificant).

This check would work, but needs a bit comment, like:

In the case of 'example.org. IN CNAME    ; hello', the Target name consists out of a single newline (all other space is discarded). Even though DNS is 8-bit clean and \n could/should be valid, this is clearly not what the writer intended, so we check this here. Not that [IsDomainName] does not have this check.

[miekg]

Suggested change

	"bad.example.org. HTTPS 10 ; bad https",
	"bad.example.org. CNAME"",

[taoso]

This may not possible because the current implementation support RR with empty RDATA to create zero RR value.

[miekg]

yeah, but not all I think? I get an error with A records here e.g. So I guess that is sorta half baked, and could be dropped. Downside is that you sometimes do want those records, and a neat way of making them would be nice.

[taoso]

The _, err = NewRR("miek.nl. IN A ") triggers error because it has the dedicate check logic in the parse funciton.

dns/scan_rr.go

Lines 104 to 106 in 8d0c412

	if rr.A == nil || !isIPv4 || l.err {
	return &ParseError{err: "bad A A", lex: l}
	}

For all RR that need domain_name target, e.g. CNAME/MX/SRV/CNAME, we have two choose to fix this issue:

• add dedicated domain check logic in their own parse function
• add dedicated domain check to the toAbsoluteName function which is used by all the related RR

BTW, this check is not CNAME only. It may not appropriate to only mention CNAME in your proposed comment

In the case of 'example.org. IN CNAME    ; hello', the Target name
consists out of a single newline (all other space is discarded).
Even though DNS is 8-bit clean and \n could/should be valid, this is
clearly not what the writer intended, so we check this here.
Not that [IsDomainName] does not have this check.

[miekg]

The _, err = NewRR("miek.nl. IN A ") triggers error because it has the dedicate check logic in the parse funciton.

yep meaning this functionality was incomplete at best. Just need to think of a better way of doing this, also see #1601

[miekg]

empty rdata with NewRR should not be retained, also see #1642