Fix #2162: use getaddrinfo instead of curl to preload NSS #2224
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
How sure are you that this is equivalent? I really should've constructed a test when I first put together the "fix" 😄but now I can't remember the exact circumstances with it. I think it has something to do with turning sandboxes on, while using In principle though, this looks pretty good and seems better than what I did. |
|
Well, I can't reproduce the bug with my patch. |
|
I guess I just mean you were seeing the bug without your patch and without the curl invocation? If so, that sounds great. |
|
No, I didn't try reproducing the bug without the patch. I'll try. |
|
I also can't reproduce this without the |
|
Oh well, let's just merge and see. |
|
Details on the original issue (for preloadNSS) would be useful. I'm concerned about this fix--there's a reason networking libraries and such have caches for DNS lookups and perform these operations asynchronously... Notably getaddrinfo is a blocking call, and is essentially unbounded in how long it might take... I don't have a great answer for what should be done instead, as it's unclear what this should be solving anyway. I trust there is, or was, an issue that we should fix but:
If we can identify what the problem is, perhaps we can find a better way to solve it...? (If not it seems this should just be deleted, and either everything will be fine or it will identify what needs fixing) |
|
Not sure why I didn't file an issue at the time, but here's some more context I dug up from some IRC logs: Trying to dig up more context on when this arises from the logs. |
|
Okay, here's much more context: |
|
So it boils down to: If you use |
|
Thank you very much! I also found this: https://nixos.org/nix-dev/2008-April/000684.html |
|
Hmm, sounds reasonable. I can't see any mention of reacting to the final dot in |
|
If I understand correctly this is for the case where nscd is disabled, yes? Finally reproduced this (when removing It looks like glibc itself in its testsuite does this: https://sourceware.org/git/?p=glibc.git;a=blob;f=resolv/tst-resolv-res_init-skeleton.c;h=a5061e6d4fb98311e2412d845b2aa01c6ae16202;hb=HEAD#l320 Tested with/without to confirm it works. I consider it a feature that this only preloads DNS and not anything else, but maybe I'm missing some use-cases. |
|
The issue with what you're doing is that I don't think the set of libraries is necessarily fixed, and is instead determined by nsswitch.conf. For example, if you use systemd-machined, you can have an nsswitch module for doing easy name resolution against your machine names, implemented by another loadable module. My preferred solution would be to make the cc @edolstra in case you have any ideas, since it might not be obvious that this conversation is continuing in this closed ticket. Speaking of that, @dtzWill perhaps we should open another ticket explaining the situation and soliciting suggestions? |
|
Any requirement to modify glibc probably won't solve this completely anytime soon. I think we need this to work even on various distro glibc versions and NSS configurations. |
|
Yeah, I acknowledged that 😄
I welcome better suggestions though! |
|
BTW, having a .invalid name that resolves seems like a violation of RFC 2606, so we could consider a local (mis)configuration issue. |
|
Don't some shady ISPs resolve everything to their "helpfu"l (ad-laden) search page? I remember I had to opt out of it a while back on my Verizon connection 😦 |
|
On DNS level it attempts to resolve EDIT: shady ISPs that put ads or whatever instead of non-existing names are a completely different level. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, a curl download of an invalid domain is used to trigger NSS library preload (for the sandbox).
This is used without a trailing
.(and url strips out the trailing.s), meaning this domain could resolve. In my case, it would take 1.5 minutes to time out.Hopefully this retains the preloading functionality while removing the delays.