1769293 - Add API PR_GetPrefLoopbackAddrInfo

Status: REOPENED

(NSPR :: NSPR, enhancement)

Description

[Kai Engert [:KaiE:]]

We need a function PR_GetPrefLoopbackAddrInfo that helps with servers, that only want to listen on a single IP address of a single family. An example is the NSS test tool selfserv.

The function will make a stable decision which family should be used. This allows separate server and client application to make the same decision for listening and connecting.

See the discussion in bug 636504 from May 2022. The bug was actually offtopic for this need.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Bug 1769293 - Add API PR_GetPrefLoopbackAddrInfo. r=rrelyea

Comment 2

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/projects/nspr/rev/2be45a781ba18e5904c1eed4dc0e1d66248d303c

Comment 3

[Robert Relyea]

OK, so I'm now processing the rebase for RHEL and ran into the following issues:

1. There are a number of 'coverity'* issues with the nspr patch. These issues are probably theoretical, but the code is structured such that it expects them to be potentially real. If the addr entry is NULL, then aNetAddr never gets set, so the later test code could be doing tests on uninitialized data (or the previous time through the loop).
2. while fixing this, I noticed that the code is clearing result based on the length of the the addr, not aNetAddr. This means the resulting code will clear out any previously set results if the later entries in the loop are smaller than the previous ones (which does, in fact, happen).
3. If I fix the previous error, NSS test cases start to fail where they weren't before. This is because PR_GetAddrPrefLoopbackAddrInfo is returning the wild card address rather then the single address. The NSS test cases all use -4 to force tstclnt to use the IPV4 interface. For the server, we in fact want the wild card (::0) to be returned rather than the loopback address (::1) so that both IPV4 and IPV6 can both connect.

Downstream, I've fixed the coverity issues and used AI_PASSIVE for PR_GetAddrPrefLoopbackAddrInfo(). That's not a complete solution, but it preserves what the code is effectively doing now. What we want is a parameter to PR_GetAddrPrefLoopbackAddrInfo, in which we pass AI_PASSIVE for the selfserv, and not for tstclnt. The question is do we want just a bool (server/client), or do we want to pass the hints.flags in wholesale (server would pass AI_PASSIVE, clients would pass '0', other could pass other flags as desired).

I'll attach the downstream patches to this bug for reference.

bob

Comment 4

[Robert Relyea]

Attachment: nspr-4.34-fix-coverity-loop-issue.patch

Coverity bug fixes.

Comment 5

[Robert Relyea]

'coverity'* from comment 3

• we use multple scanners, including clang in our 'coverity' scans. I think these issues were found with clang.

Comment 6

[Robert Relyea]

Attachment: nspr-4.34-server-passive.patch

Comment 7

[Kai Engert [:KaiE:]]

Bob, this API is specifically designed to make a choice. That's implied in the naming of the function, "Pref" means preferred.

I have the impression that your explanation is contrary to the behavior.
You said above:

PR_GetAddrPrefLoopbackAddrInfo is returning the wild card address rather then the single address.

I don't believe that. According to the manual page of gettaddrinfo, it's the opposite of what you say.
It says, if the flag AI_PASSIVE is absent, as in the current implementation, then a specific socket will be returned, suitable for connect.
It also says, if the flag AI_PASSIVE is set, you get a wildcard.

I think this function should NOT return a wildcard.
If you want selfserv to listen on all stacks (wildcard), I see no point in calling a function that helps you make a choice.

Comment 8

[Kai Engert [:KaiE:]]

(In reply to Robert Relyea from comment #3)

The NSS test cases all use -4 to force tstclnt to use the IPV4 interface.

This seems wrong. If you are on a system that has IPv6, only, it won't work.

Is it possible that this was the cause of the original problem???
Did we get a failure, because a system had only IPv6, but tstclnt attempted to connect to nonexisting 127.0.0.1 because of -4 ?

Comment 9

[Kai Engert [:KaiE:]]

Comment on attachment 9281662 [details] [diff] [review]
nspr-4.34-fix-coverity-loop-issue.patch

Review of attachment 9281662 [details] [diff] [review]:
-----------------------------------------------------------------

::: pr/src/misc/prnetdb.c
@@ +2214,5 @@
>              while (ai && ai->ai_addrlen > sizeof(PRNetAddr))
>                  ai = ai->ai_next;
>  
> +            if (!ai) {
> +                break;

That's also not correct. If this happens on first iteration, then the function will return PR_SUCCESS, despite no data having been copied to result.

Comment 10

[Kai Engert [:KaiE:]]

(In reply to Robert Relyea from comment #3)

2. while fixing this, I noticed that the code is clearing result based on the length of the the addr, not aNetAddr. This means the resulting code will clear out any previously set results if the later entries in the loop are smaller than the previous ones (which does, in fact, happen).

Yes, there is a bug, No, that's not the correct fix. It could potentially destroy previously valid data already copied to result.

The code I wrote was inspired by function PR_EnumerateAddrInfo.

Looking at that function, the correct fix is to clear the trailing part of temporary buffer aNetAddr. (We are copying the input into aNetAddr, for potentially using it as a result, and we must ensure our temporary space inside aNetAddr doesn't contain previous data.)

(Sorry that 10-year-ago-me wrote such a crappy patch.)

Comment 11

[Kai Engert [:KaiE:]]

Comment on attachment 9281663 [details] [diff] [review]
nspr-4.34-server-passive.patch

I don't agree with this change, as explained. I think this function is designed to make a choice, and return a specific address (this is my understanding of what the current code does).

This function should NOT return a wildcard. You shouldn't need this function if you want to listen on the wildcard address. If you need a wildcard address, we must introduce an additional API.

Comment 12

[Kai Engert [:KaiE:]]

Comment on attachment 9281662 [details] [diff] [review]
nspr-4.34-fix-coverity-loop-issue.patch

This patch has some good parts, but I think there are also changes needed.
For simplicity, I'll mark this patch as r-, and will attach a new patch based on it (which has lots of similarities).

To make it easier to handle the review of whitespace/indenting changes, I'll attempt to submit a phabricator patch.

Comment 13

[Kai Engert [:KaiE:]]

Attachment: Bug 1769293 - Follow-up correctness fixes. r=rrelyea

Comment 14

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:KaiE:) from comment #11)

If you need a wildcard address, we must introduce an additional API.

Clarification of my words:

If you cannot figure out how to change selfserv to listen on a wildcard address, possibly because we don't have an API that returns it, then we must introduce an additional API.

It seems like this might have been the original intention of bug 636504

Comment 15

[Kai Engert [:KaiE:]]

If we get this done quickly, maybe by tomorrow, it could be included in 4.34.1 (bug 1769293)

Comment 16

[Robert Relyea]

OK, so here's the deal:
I can never get the server to work unless I use AI_PASSIVE. I've tried it fixed, without AI_PASSIVE and the code for the client and the -4 removed from the tests and I get failures once I fixed the bug that caused the code to always return AI_PASSIVE (effectively). With AI_PASSIVE on the server, everything works whether we connect with -4 or not and it works for any tst client connecting. (I do have a patch that removes the -4 flag from the tests and makes it an environment option... not -4 is only on tstclient, we don't have explict operators on selfserv).

So yes, we need code that returns a wild card address.

Note: since you made a new API, and because of a bug that API always returned AI_PASSIVE (because of a bug), for binary compatiblity reasons I made it explicit. I'm OK with a new API that either takes a parameter or returns a Loopback without AI_PASSIVE as a new function.

Bob