Malformed DNS RRs shall result in parse errors

[snar]

For now, when DNS RR is malformed it results in 'parse ok, however, some records are not parsed':

3> inet_res:resolve("kinolupa.net", in, aaaa, [], 5000).
{ok,#dns_rec{header = #dns_header{id = 1,qr = true, opcode = query,aa = false,tc = false, rd = true,ra = true,pr = false,rcode = 0}, qdlist = [#dns_query{domain = "kinolupa.net",type = aaaa, class = in}], anlist = [#dns_rr{domain = "kinolupa.net",type = aaaa, class = in,cnt = 0,ttl = 600, data = <<171,22,127,201>>, tm = undefined,bm = [],func = false}, #dns_rr{domain = "kinolupa.net",type = aaaa,class = in, cnt = 0,ttl = 600, data = <<171,22,127,201>>, tm = undefined,bm = [],func = false}], nslist = [],arlist = []}}

As you can see, some broken DNS server returned two malformed (too short) AAAA records that
erlang was not able to parse and so they are returned as binary <<171,22,127,201>>.
With this pull applied, such broken records results in {error, formerr}.

[CLAassistant]

All committers have signed the CLA.

[RaimoNiskanen]

Thank you for finding this!

Although your change seems correct and fairly minimal, I did not like what it does to the structure of the code. Now all RR types have to have code in two distinct clauses for decode_data/4; for decode, and for format error. And the clauses are rather far apart.

So I did a rewrite, which, to avoid repeating throw(?DECODE_ERROR) in umpteen places, instead uses a match macro that does the throw. And I fixed some more structural issues. See the commit comment.

The diff is rather large, but I hope the code becomes more maintainable.

If you hate it I can do this on a branch of my own instead of piggybacking on your PR.

[RaimoNiskanen]

I discovered I had used an old name for the macro in the commit comment, so I force pushed a new.

[snar]

On Fri, Aug 27, 2021 at 05:42:24AM -0700, Raimo Niskanen wrote: Thank you for finding this!

Yeah, I was quite surprized when code that worked without problems for some years suddenly broken and then finding reason for this breakage..

Although your change seems correct and fairly minimal, I did not like what it does to the structure of the code. Now all RR types have to have code in two distinct clauses for decode_data/4; for decode, and for format error. And the clauses are rather far apart. So I did a rewrite, which, to avoid repeating throw(?DECODE_ERROR) in umpteen places, instead uses a match macro that does the throw. And I fixed some more structural issues. See the commit comment. The diff is rather large, but I hope the code becomes more maintainable. If you hate it I can do this on a branch of my own instead of piggybacking on your PR.

No problem: correct code can't be ugly.

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. *

[RaimoNiskanen]

@snar: Can you test the code with my rewrite to verify that it seems to work as expected?

[snar]

On Mon, Aug 30, 2021 at 05:35:31AM -0700, Raimo Niskanen wrote: @snar: Can you test the code with my rewrite to verify that it seems to work as expected?

Sure, already done: known 'bad' query results in {error, formerr} as expected: 17> inet_res:resolve("kinolupa.net", in, aaaa, [{nameservers, [{{47,74,31,16}, 53}]}], 5000). {error,formerr} smoke tests for 'good' queries results in perfectly normal answers.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. *

[RaimoNiskanen]

Good to know! Thank you for your pull request!