[bugfix] Reject hexadecimal blobs with odd number of characters

[miodvallat]

Short description

Until about 2011, hexadecimal input as ascii strings were required to have an even number of characters, in order to make a correct number of bytes. Then changes were made to allow whitespace in these strings, or a dangling single hex char was accepted as the high-order part of a complete byte, i.e. "ABCDE" would be parsed as "ABCDE0".

This PR brings back the exception which used to be raised for an odd number of meaningful characters, but does not change the rest of the logic (i.e. you can still write "AB_CD_EF" if you want).

Checklist

I have:

• read the CONTRIBUTING.md document
• compiled this code
• tested this code
• included documentation (including possible behaviour changes)
• documented the code
• added or modified regression test(s)
• added or modified unit test(s)

[Habbie]

Neat!

If you move fixes #12650 into the commit message body it should automatically link this PR to the ticket. I also predict that clang-tidy is going to ask you to put some braces on this code now that you touched it.

[miodvallat]

Ah, right. I'll amend the message (and maybe the commit...) after the CI completes.

[coveralls]

Pull Request Test Coverage Report for Build 12194870868

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
• 82 unchanged lines in 18 files lost coverage.
• Overall coverage decreased (-0.02%) to 64.689%

---

Files with Coverage Reduction	New Missed Lines	%
pdns/misc.cc	1	63.49%
pdns/dnsdistdist/dnsdist-crypto.cc	1	76.01%
pdns/recursordist/negcache.hh	2	88.24%
pdns/rcpgenerator.cc	2	90.28%
pdns/recursordist/test-syncres_cc2.cc	3	88.91%
pdns/axfr-retriever.cc	3	67.48%
pdns/recursordist/syncres.cc	3	79.42%
pdns/recursordist/recpacketcache.hh	3	89.55%
pdns/recursordist/rec-main.cc	3	62.35%
pdns/dnsdistdist/dnsdist.cc	4	68.38%

Totals
Change from base Build 12116232290:	-0.02%
Covered Lines:	125845
Relevant Lines:	163640

---

💛 - Coveralls

[omoerbeek]

The recursor regression test failure is due to a dependency on external authoritative servers in combination with lousy GH UDP connectivity. One day we will get rid of those dependencies. I'll restart that test.

[miodvallat]

I was quite convinced this had nothing to do with that change but couldn't infer that from the logs.

On the other hand, I am worried about the coverage results because this shouldn't cause any change in coverage - this particular case is not tested yet (because I am still arm-wrestling the regress tests at the moment and I have yet to win.)

[omoerbeek]

I was quite convinced this had nothing to do with that change but couldn't infer that from the logs.

On the other hand, I am worried about the coverage results because this shouldn't cause any change in coverage - this particular case is not tested yet (because I am still arm-wrestling the regress tests at the moment and I have yet to win.)

Don't be too worried, there are cases where non-determinism is going on, so the tests vary a bit in coverage per run.

As for regress test wrestling: sadly these parts carry a lot of legacy load. While we have an ongoing effort to clean up the code base, the various test mechanisms we have are a bit of a neglected area in that respect.

[omoerbeek]

As for a test: I'd go for something lile

  BOOST_CHECK_THROW(DNSRecordContent::make(QType::NSEC3PARAM, QClass::IN, "1 0 12 a"), RecordTextException);

in an appropriate spot in test-dnsrecords_cc.cc

[miodvallat]

Thanks for the hint!