Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prevent stripping trailing dot for resolving #3222

Closed
wants to merge 3 commits into from

Conversation

thz
Copy link
Contributor

@thz thz commented Nov 2, 2018

This change removes the stripping of trailing dots in hostnames for the aspect of name resolving.
It does not change hostnames for SNI or http Host header.

This allows using a trailing dot to denote a fully qualified name with DNS.

Fixes: #3022

lib/url.c Outdated Show resolved Hide resolved
lib/url.c Outdated Show resolved Hide resolved
lib/url.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member

bagder commented Nov 5, 2018

Test 20 and 1322 both have memory issues reported on travis. One leak, one illegal free.

@thz
Copy link
Contributor Author

thz commented Nov 5, 2018

@bagder, As I eventually only moved the resolving it seems to me that I am somehow triggering an pre-existing issue.
But I am still investigating. In case you have an idea where that is coming from, a hint would be highly appreciated.

@bagder
Copy link
Member

bagder commented Nov 6, 2018

I only moved the resolving

You moved where the resolving starts. Most libcurl builds will resolve host name asynchronously so when resolve_server() returns it might not be done yet. You're then removing the trailing dot when the resolver function sometimes hasn't completed.

I don't think we can can use the same strings for both with and without trailing dot purposes unless we're really sure that those buffers won't be needed again in their original pristine versions.

@thz
Copy link
Contributor Author

thz commented Nov 6, 2018

All right. I will go for a dedicated name for resolving purposes then.

@thz thz force-pushed the thz/no-dotstrip-for-resolve branch from 57c75a5 to 9306efe Compare November 13, 2018 08:47
@thz
Copy link
Contributor Author

thz commented Nov 13, 2018

It seems to me that the tests are running into the issue previously found here:
https://bugs.launchpad.net/ubuntu/+source/eglibc/+bug/1719959
There seems to be a known eglibc-2.19 leak in getaddrinfo when fed with a "bad address".
I am not exactly sure why this did not surface before.

@thz
Copy link
Contributor Author

thz commented Nov 13, 2018

I can reproduce the issue now in a docker container with ubuntu-14.04 userland. Tracking it down...

@thz
Copy link
Contributor Author

thz commented Nov 13, 2018

Test 20 fails because of the getaddrinfo memory leak found in eglibc-2.19 (trusty is used).
I created a minimal case for this issue at https://github.com/thz/getaddrinfo-leak to show this.

A mitigation might be to change test20 to a IPv4 only test (it is about non-existing names anyways), because the bug is only triggered with PF_UNSPEC.
Alternatively a newer travis environment/userland (xenial would be enough) would prevent this bug from surfacing too.

I did not look into the details for test 1322 yet.

(CC: @bagder)

@thz
Copy link
Contributor Author

thz commented Nov 13, 2018

The reason why this did not happen before is that only some names trigger it when resolving. Two examples:

  • a name which yields no existing address (NXDOMAIN for dns), which has a trailing dot
  • a name which yields a mix of A and AAAA records

Test 20 could also be mitigated by removing the trailing dot in data/test20.

@thz
Copy link
Contributor Author

thz commented Nov 13, 2018

Test1322 failing is of similar nature. Here the old eglibc-2.19 (ubuntu-14.04 used in travis) is doing free() in an atexit handler and surfaces an invalid free in valgrind. Forcing ipv4 is again a mitigation. Going for a newer userland in travis would prevent this as well.

@thz thz force-pushed the thz/no-dotstrip-for-resolve branch from f0ae7da to 77ef92b Compare November 13, 2018 19:46
@thz
Copy link
Contributor Author

thz commented Nov 13, 2018

Test 1526 on Visual Studio 12 seems flaky.

@thz
Copy link
Contributor Author

thz commented Nov 13, 2018

Now a osx clang build failed during build setup. Is there a way to trigger a rebuild without pushing phony commits?

@thz thz changed the title WIP: prevent stripping trailing dot for resolving prevent stripping trailing dot for resolving Nov 14, 2018
@thz
Copy link
Contributor Author

thz commented Nov 14, 2018

@bagder all issues addressed. Please let me know if the test mitigation is acceptable.

@bagder
Copy link
Member

bagder commented Nov 20, 2018

Sorry for being slow. Can you please squash this set of commits into just a series of suitable commits without fixups (and force-push)? That simplifies reviewing greatly and is the way we want to merge them anyway!

@thz thz force-pushed the thz/no-dotstrip-for-resolve branch from 6d3df1a to 715bbf2 Compare November 21, 2018 08:59
@thz
Copy link
Contributor Author

thz commented Nov 21, 2018

apt-get install failed due to some unreachable ppa... Completely unrelated. Is it possible to just trigger a rebuild?

@thz
Copy link
Contributor Author

thz commented Nov 21, 2018

@bagder done rebasing/squashing.

thz added 3 commits November 21, 2018 21:50
Delays stripping of trailing dots to after resolving the hostname.
The tests 20 and 1322 are using getaddrinfo of libc
for resolving. In eglibc-2.19 there is a memory leakage
and invalid free bug which surfaces in some special
circumstances (PF_UNSPEC hint with invalid or non-existent
names). The valgrind runs in testing fail in these
situations.

As the tests 20/1322 are not specific on either protocol
(IPv4/IPv6) this commit changes the hints to IPv4 protocol
by passing `--ipv4` flag on the tests' command line.
This prevents the valgrind failures.
@thz thz force-pushed the thz/no-dotstrip-for-resolve branch from 715bbf2 to 097fa6e Compare November 21, 2018 20:50
@thz
Copy link
Contributor Author

thz commented Nov 22, 2018

@bagder ping. Used another rebase for getting a retest. All green now.

@bagder
Copy link
Member

bagder commented Nov 22, 2018

Awesome, thanks for your hard work on this!

@bagder bagder closed this in 5b4cce2 Nov 22, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Feb 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants