libc/tests: add getaddrinfo test

[glebius]

Summary

A test suite for getaddrinfo(3) written in C. Unlike NetBSD test, this
one will be mostly focused on what the API should return when something
isn't good with your DNS. Test emulates bad DNS servers in resolv.conf
intercepting fopen(2) and emulates downed network intercepting send(2).

Initial version covers three main scenarios: all good, server(s) timed
out, network down. For each scenario we test hostname with trailing dot
and without, since libc resolver uses quite different code paths,
sometimes even yielding with different error codes. All current error
codes in the test are what out libc returns right now, meaning the test
documents what we have, not what there should be.

[glebius]

Added all reviewers of D37139

[bz]

lib/libc/tests/net/getaddrinfo/getaddrinfo2.c

41 ↗

(On Diff #152406)

I cannot imagine that this will be long-term stable. If need I wonder if clusteradm could add a specific AAAA-only record pointing to an 100::/64 address.

You could also try IPv4-only with AF_INET6? ipv4only.arpa. exists.

Also you do understand that these tests relying on a positive reply will fail - if internet is down, if the machine running tests has no outside connectivity, ... very common for lab networks in companies. I'll add lwhsu to see if he knows if jenkins can access public DNS.

43 ↗

(On Diff #152406)

s/doesn't/don't/ They still exist btw. which are not in public use? which are not announced globally?

The way to do this is to bind them locally to a disc0 interface.
People tend to run experiments and sometimes advertise them and sometimes don't.
Also don't rely on whether your upstream router does send ICMP or not. Mileage may vary.

[glebius]

I was actually looking into reviewing the actual test cases, e.g. condition A -> return code B, not the DNS names or nets that are used for the test.

lib/libc/tests/net/getaddrinfo/getaddrinfo2.c

41 ↗

(On Diff #152406)

I'm totally fine with improving these constants. If anybody have better ideas here - feel free to improve.

I will add ipv4only.arpa test.

The test instances that don't have internet access would need to disable this test (part of test suite). How otherwise would you write test that test DNS resolution?

43 ↗

(On Diff #152406)

So you say TEST-NET-3 was in use before? I can take a different test net then. I don't want to add routes to disc0 for two reasons:

1. This will make the test have global effect and test runner would need to not run any other tests in parallel. Given that this test takes time to run (and possible with more test cases in will take more) that would increase overall test run by several minutes.
2. This will make the test require root priveleges.

The above two problems, IMHO, are bigger than potential flakiness of the test, that may happen if several constellations combine: unfiltered TEST-NET-3 announce with a responding host.

[bz]

In D49410#1126808, @glebius wrote:

Okay, in that case just keep the test net etc as-is and we'll accept the flakyness.

[glebius]

Add ipv4only.arpa to the nofamily test.

[kib]

The test depends on the another implementation detail, namely that resolver opens the resolv.conf file each time it needs to do the resolution. I suspect that much better way to exercise is to fork/exec a fresh test process, to not depend on this detail.

lib/libc/tests/net/getaddrinfo/getaddrinfo2.c

28 ↗

(On Diff #152418)

Is this inclusion still needed?

39 ↗

(On Diff #152418)

Why not static const char []?

51 ↗

(On Diff #152418)

Why not automate resconflen with the use of strlen()?

58 ↗

(On Diff #152418)

You should check for orig == NULL and at least print the dlerror() message.

59 ↗

(On Diff #152418)

Either remove {} around the then block, or add them around the else block. Or better, remove the 'else' line at all.

67 ↗

(On Diff #152418)

What about sendmsg/sendto? As-is, the interceptor relies too hard on the detail of the implementation.

102 ↗

(On Diff #152418)

I suggest to print the numeric value for EAI_NONAME as well.

104 ↗

(On Diff #152418)

Can we please avoid such modifications? Either add badname_without_dot (pref), or do strdup()/strrchr(). But IMO the static string is better.

[glebius]

Address Kostik's review.

[glebius]

lib/libc/tests/net/getaddrinfo/getaddrinfo2.c

67 ↗

(On Diff #152418)

What about sendmsg/sendto? As-is, the interceptor relies too hard on the detail of the implementation.

That's true, but what can we do? As long as test is bundled with the library, IMHO, that's fine.

[kib]

lib/libc/tests/net/getaddrinfo/getaddrinfo2.c

74 ↗

(On Diff #152428)

I pointed one case in the previous batch, but please do it everywhere: either use {} around both then/else blocks, or not use them for both.

117 ↗

(On Diff #152428)

I suggest to split this declaration into two, and make hints static const.

67 ↗

(On Diff #152418)

Provide the interposers for sendmsg/sendto as well from the start?

[kp]

lib/libc/tests/net/getaddrinfo/getaddrinfo2.c

41 ↗

(On Diff #152406)

How otherwise would you write test that test DNS resolution?

Scapy can be convinced to pretend to be a DNS server. That'd also give us the ability to generate DNS replies where we guarantee dnssec records (or their absence) or even malformed DNS replies in the future.

Example code: https://jasonmurray.org/posts/2020/scapydns/

[glebius]

lib/libc/tests/net/getaddrinfo/getaddrinfo2.c

41 ↗

(On Diff #152406)

Scapy can be convinced to pretend to be a DNS server.

That would be a cool addition of course! But ATM I don't have energy for this. I already invested some into this test mostly to provide solid backing for proposed changes for EAI_AGAIN.

[glebius]

• more curly braces
• declare hints as const
• retitle revision, we actually don't have a conflict with NetBSD test

[glebius]

lib/libc/tests/net/getaddrinfo/getaddrinfo2.c

117 ↗

(On Diff #152428)

I don't see any value in making them static, but added constness.

67 ↗

(On Diff #152418)

Provide the interposers for sendmsg/sendto as well from the start?

I don't agree. That would add a code to the test that was never tested itself. I'd prefer to have a highly hypothetical duty to repair the test in the future, rather than trying to predict how exactly it may be broken in the future.

[kib]

lib/libc/tests/net/getaddrinfo/getaddrinfo2.c

117 ↗

(On Diff #152428)

There is no reason to initialize the structure in runtime instead of having the compiler filling it.

40 ↗

(On Diff #152418)

Is there technical reason to not have these strings const?

67 ↗

(On Diff #152418)

So implement send() as a trivial wrapper around sendmsg().

[glebius]

lib/libc/tests/net/getaddrinfo/getaddrinfo2.c

40 ↗

(On Diff #152418)

Missed that.

[glebius]

lib/libc/tests/net/getaddrinfo/getaddrinfo2.c

40 ↗

(On Diff #152418)

Nope, was confused, thought I missed the hostname strings. The resolv.conf strings cannot be const because they are passed to fmemopen().

P.S. Hmm. these actually are hostnames. Something wrong with phabricator - last revision I pushed has all hostnames as const.

[glebius]

Make hints declarations static.

[glebius]

lib/libc/tests/net/getaddrinfo/getaddrinfo2.c

67 ↗

(On Diff #152418)

Sorry, I don't understand neither plan nor I agree with the intention for that.

[kib]

lib/libc/tests/net/getaddrinfo/getaddrinfo2.c

67 ↗

(On Diff #152418)

This would allow to override both send and sendmsg with min efforts, and have both override code tested. Most important, this would leave one less mine for future updates to the resolver, where bugs in the test suite needs to be fixed.

But ok.