dns_msg: skip RDLENGTH_LENGTH field when skipping record

[benpicco]

Contribution description

dns_msg skips unknown records, but the length record length field was only added when the record was not skipped - this caused the next record to be started 2 bytes short.

Testing procedure

enable DNS with DNS64 server

USEMODULE += sock_dns
USEMODULE += auto_init_sock_dns
CFLAGS += -DCONFIG_AUTO_INIT_SOCK_DNS_SERVER_ADDR=\"2001:67c:2b0::6\"
CFLAGS += -DCONFIG_DNS_MSG_LEN=256 # response msg is 202 bytes

We can resolve an IPv4 host behind NAT64 now 🎉

2024-09-09 12:00:59,181 # > ping global.azure-devices-provisioning.net
2024-09-09 12:00:59,341 # 12 bytes from 2001:67c:2b0:db32::1432:418d: icmp_seq=0 ttl=105 time=84.760 ms
2024-09-09 12:01:00,336 # 12 bytes from 2001:67c:2b0:db32::1432:418d: icmp_seq=1 ttl=105 time=80.033 ms
2024-09-09 12:01:01,337 # 12 bytes from 2001:67c:2b0:db32::1432:418d: icmp_seq=2 ttl=105 time=79.999 ms
2024-09-09 12:01:01,337 # 
2024-09-09 12:01:01,338 # --- global.azure-devices-provisioning.net PING statistics ---
2024-09-09 12:01:01,339 # 3 packets transmitted, 3 packets received, 0% packet loss
2024-09-09 12:01:01,340 # round-trip min/avg/max = 79.999/81.597/84.760 ms

Issues/PRs references

fixes #20355

[riot-ci]

Murdock results

✔️ PASSED

0463b27 unittests: add test for dns_msg

Success	Failures	Total	Runtime
10197	0	10197	16m:24s

Artifacts

• Documentation preview

[miri64]

Can you add your test vector to https://github.com/RIOT-OS/RIOT/blob/master/tests/net/gnrc_sock_dns/tests-with-config/01-run.py to prevent future regressions, please? I think just adding the hexdump in a scapy DNS object should be enough, but, as we learned during the summit: It always pays to learn a little scapy for testing network stuff ;-).

[benpicco]

Ugh I think that test is somewhat broken. All requests after the first test_success result in a timeout.
The test still succeeds, because it expects all but the first test to fail, but if you do

--- a/tests/net/gnrc_sock_dns/tests-with-config/01-run.py
+++ b/tests/net/gnrc_sock_dns/tests-with-config/01-run.py
@@ -296,6 +296,7 @@ def testfunc(child):
 
         run(test_success)
         run(test_timeout)
+        run(test_success)
         run(test_too_short_response)
         run(test_qdcount_too_large1)
         run(test_qdcount_too_large2)

you'll find that the test isn't testing anything at all. (also on master)

[miri64]

Ugh I think that test is somewhat broken. All requests after the first test_success result in a timeout.
The test still succeeds, because it expects all but the first test to fail, but if you do […] you'll find that the test isn't testing anything at all. (also on master)

Yeah, I was surprised that there is no dedicated test for dns_msg either. But I guess that has historical reasons, since it used to be part of sock_dns. Anyway, would be nice to have some test to prevent regression.

[benpicco]

I created unittests/tests-dns_msg to track regressions.
The test fails on master but succeeds with this PR.

It currently only contains positive tests, but could/should be extended with malicious data in the future to better stress the parser.

But IMHO that's out of the scope of a bugfix PR.

[benpicco]

rebased to to master because of #20860

[miri64]

IMHO better to use example values in the tests (actual values have the tendency to get stuck, e.g.), other than that, I would ACK.

Addressed it myself.

[benpicco]

Thank you for addressing and documenting the test vectors!

[miri64]

Fixed a spelling mistake in the name 😅