Re: [PATCH 01/12] Simplify allocations and fix merge and continue actions [BZ #28931]

[Carlos O'Donell <carlos@redhat.com>]

On 3/8/22 08:52, Siddhesh Poyarekar wrote:
> On 08/03/2022 15:37, Siddhesh Poyarekar via Libc-alpha wrote:
>> Allocations for address tuples is currently a bit confusing because of
>> the pointer chasing through PAT, making it hard to observe the sequence
>> in which allocations have been made.  Narrow scope of the pointer
>> chasing through PAT so that it is only used where necessary.
>>
>> This also tightens actions behaviour with the hosts database in
>> getaddrinfo to comply with the manual text.  The "continue" action
>> discards previous results and the "merge" action results in an immedate
>> lookup failure.  Consequently, chaining of allocations across modules is
>> no longer necessary, thus opening up cleanup opportunities.
>>
>> A test has been added that checks some combinations to ensure that they
>> work correctly.  The "dns [SUCCESS=continue] dns" for example results in
>> a segfault without this fix.
>>
>> Resolves: BZ #28931
>>
>> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
>> ---
>>   nss/Makefile                |   1 +
>>   nss/tst-nss-gai-actions.c   | 242 ++++++++++++++++++++++++++++++++++++
>>   sysdeps/posix/getaddrinfo.c | 142 +++++++++++++--------
>>   3 files changed, 333 insertions(+), 52 deletions(-)
>>   create mode 100644 nss/tst-nss-gai-actions.c
>>
>> diff --git a/nss/Makefile b/nss/Makefile
>> index 552e5d03e1..43a0b9defe 100644
>> --- a/nss/Makefile
>> +++ b/nss/Makefile
>> @@ -78,6 +78,7 @@ tests += tst-nss-files-hosts-multi
>>   tests += tst-nss-files-hosts-getent
>>   tests += tst-nss-files-alias-leak
>>   tests += tst-nss-files-alias-truncated
>> +tests += tst-nss-gai-actions
>>   endif
>>     # If we have a thread library then we can test cancellation against
>> diff --git a/nss/tst-nss-gai-actions.c b/nss/tst-nss-gai-actions.c
>> new file mode 100644
>> index 0000000000..b5ba5f0138
>> --- /dev/null
>> +++ b/nss/tst-nss-gai-actions.c
>> @@ -0,0 +1,242 @@
>> +/* Test continue and merge NSS actions for getaddrinfo.
> 
> Looks like DNS tests don't work under the patchwork trybot container.
> I'll mark the "dns [SUCCESS=continue] dns" tests as FAIL_UNSUPPORTED
> when it encounters a temporary failure in name resolution.

No new tests should rely on external DNS being available during testing.

The trybots must be able to run disconnected and fully isolated from the network
from an operational security perspective.

You can and should provide either a stub NSS resolver that returns results for DNS
that just feeds that data back to you.

This is an indication of an incomplete test, in that we need to be able to control
all inputs to the test, otherwise this test is unreliable at best and will spurious
fail when DNS goes down.

-- 
Cheers,
Carlos.