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

inet_res: Make host name lookups case-insensitive #763

Merged
merged 1 commit into from Nov 5, 2015

Conversation

Projects
None yet
7 participants
@weiss
Contributor

weiss commented Jun 2, 2015

Don't let inet_res:getbyname and inet_res:gethostbyname calls return {error, nxdomain} if the host name capitalization in the DNS response differs from the request, like in this example:

1> inet_res:gethostbyname("erlang.org").
{ok,{hostent,"erlang.org",[],inet,4,[{192,121,151,106}]}}
2> inet_res:gethostbyname("Erlang.ORG").
{error,nxdomain}

This PR partly reverts commit 8152505. Up to that commit, only the initial lookup of a given host name was case-sensitive. Once the result was cached, subsequent lookups were case-insensitive. That commit changed the cache access to be case-sensitive as well. This PR makes sure all lookups are performed in a case-insensitive manner.

@OTP-Maintainer

This comment has been minimized.

Show comment
Hide comment
@OTP-Maintainer

OTP-Maintainer Jun 3, 2015

Patch has passed first testings and has been assigned to be reviewed


I am a script, I am not human


Patch has passed first testings and has been assigned to be reviewed


I am a script, I am not human


@IngelaAndin

This comment has been minimized.

Show comment
Hide comment
@IngelaAndin

IngelaAndin Jun 10, 2015

Contributor

We will consider this patch for 18.1

Contributor

IngelaAndin commented Jun 10, 2015

We will consider this patch for 18.1

@weiss

This comment has been minimized.

Show comment
Hide comment
@weiss

weiss Aug 4, 2015

Contributor

We will consider this patch for 18.1

Thanks! I guess this change is a bit of a PITA to review, but the issue affects e.g. server-to-server connections initiated by ejabberd, which just fail if the name capitalization in the DNS response differs from the request. It would be awesome if that could be fixed.

Contributor

weiss commented Aug 4, 2015

We will consider this patch for 18.1

Thanks! I guess this change is a bit of a PITA to review, but the issue affects e.g. server-to-server connections initiated by ejabberd, which just fail if the name capitalization in the DNS response differs from the request. It would be awesome if that could be fixed.

@essen

This comment has been minimized.

Show comment
Hide comment
@essen

essen Aug 5, 2015

Contributor

Cn n f,k

Loïc Hoguin
http://ninenines.eu

-------- Original Message --------
From:Holger Weiß notifications@github.com
Sent:Tue, 04 Aug 2015 16:43:36 +0200
To:erlang/otp otp@noreply.github.com
Subject:Re: [otp] inet_res: Make host name lookups case-insensitive (#763)

We will consider this patch for 18.1

Thanks! I guess this change is a bit of a PITA to review, but the issue affects e.g. server-to-server connections initiated by ejabberd, which just fail if the name capitalization in the DNS response differs from the request. It would be awesome if that could be fixed.


Reply to this email directly or view it on GitHub.

Contributor

essen commented Aug 5, 2015

Cn n f,k

Loïc Hoguin
http://ninenines.eu

-------- Original Message --------
From:Holger Weiß notifications@github.com
Sent:Tue, 04 Aug 2015 16:43:36 +0200
To:erlang/otp otp@noreply.github.com
Subject:Re: [otp] inet_res: Make host name lookups case-insensitive (#763)

We will consider this patch for 18.1

Thanks! I guess this change is a bit of a PITA to review, but the issue affects e.g. server-to-server connections initiated by ejabberd, which just fail if the name capitalization in the DNS response differs from the request. It would be awesome if that could be fixed.


Reply to this email directly or view it on GitHub.

@IngelaAndin

This comment has been minimized.

Show comment
Hide comment
@IngelaAndin

IngelaAndin Sep 9, 2015

Contributor

Have you seen PR 821? It seems to be addressing the same problem. So far I think your solution is
probably better as string:to_lower does not follow RFC 4343. However PR 821 is providing test cases.
Do you think we could combine your solution with the tests of PR 821?

Contributor

IngelaAndin commented Sep 9, 2015

Have you seen PR 821? It seems to be addressing the same problem. So far I think your solution is
probably better as string:to_lower does not follow RFC 4343. However PR 821 is providing test cases.
Do you think we could combine your solution with the tests of PR 821?

@IngelaAndin

This comment has been minimized.

Show comment
Hide comment
@IngelaAndin

IngelaAndin Sep 9, 2015

Contributor

@weiss would the new version of PR 821 solve your problem? Or do you intend to change the returned domain name to always be lower case?

Contributor

IngelaAndin commented Sep 9, 2015

@weiss would the new version of PR 821 solve your problem? Or do you intend to change the returned domain name to always be lower case?

@weiss

This comment has been minimized.

Show comment
Hide comment
@weiss

weiss Sep 9, 2015

Contributor

Do you think we could combine your solution with the tests of PR 821?

I would certainly hope the tests will be happy with my solution as well :-) I'm afraid I didn't manage to run that part of the test suite (which is the reason I didn't provide tests myself). If someone else could try running those tests, that would be awesome.

do you intend to change the returned domain name to always be lower case?

No, and as I (now) mentioned in #821, I don't think I'm doing that.

Contributor

weiss commented Sep 9, 2015

Do you think we could combine your solution with the tests of PR 821?

I would certainly hope the tests will be happy with my solution as well :-) I'm afraid I didn't manage to run that part of the test suite (which is the reason I didn't provide tests myself). If someone else could try running those tests, that would be awesome.

do you intend to change the returned domain name to always be lower case?

No, and as I (now) mentioned in #821, I don't think I'm doing that.

@stolen

This comment has been minimized.

Show comment
Hide comment
@stolen

stolen Sep 9, 2015

Contributor

The behavior of inet_db:res_hostent_by_domain here is much better than in #821.
And tests from #821 should work well (with minor fix — proxy may serve exactly one request)

Contributor

stolen commented Sep 9, 2015

The behavior of inet_db:res_hostent_by_domain here is much better than in #821.
And tests from #821 should work well (with minor fix — proxy may serve exactly one request)

@weiss

This comment has been minimized.

Show comment
Hide comment
@weiss

weiss Sep 9, 2015

Contributor

I had another look at my diff after @stolen mentioned performance in #821. Initially, I lower-cased all RRs twice: First (in res_hostent_by_domain/3) to match the user's current request against the RRs, and then (when handling add_rr/1 calls) in order to cache those RRs lower-cased.

I did this on purpose, because add_rr/1 and add_rr/5 are exported, so I wanted to make sure RRs are also cached lower-cased when those functions are not called via res_hostent_by_domain/3 but from external modules. However, right now those functions are only called from res_hostent_by_domain/3 and from the test suite, not from anywhere else within Erlang/OTP. So, I guess the test suite might be the only reason those functions are exported, and it can safely be assumed they are always called with lower-cased RRs.

Therefore, I'll now force-push a version of my patch which lower-cases the RRs just once. While at it, I'll also make sure the domain name specified in the lookup is only lower-cased once.

Contributor

weiss commented Sep 9, 2015

I had another look at my diff after @stolen mentioned performance in #821. Initially, I lower-cased all RRs twice: First (in res_hostent_by_domain/3) to match the user's current request against the RRs, and then (when handling add_rr/1 calls) in order to cache those RRs lower-cased.

I did this on purpose, because add_rr/1 and add_rr/5 are exported, so I wanted to make sure RRs are also cached lower-cased when those functions are not called via res_hostent_by_domain/3 but from external modules. However, right now those functions are only called from res_hostent_by_domain/3 and from the test suite, not from anywhere else within Erlang/OTP. So, I guess the test suite might be the only reason those functions are exported, and it can safely be assumed they are always called with lower-cased RRs.

Therefore, I'll now force-push a version of my patch which lower-cases the RRs just once. While at it, I'll also make sure the domain name specified in the lookup is only lower-cased once.

@RaimoNiskanen

This comment has been minimized.

Show comment
Hide comment
@RaimoNiskanen

RaimoNiskanen Sep 10, 2015

Contributor

I suspect that there is an irregularity in that returned aliases become lowercased (in the recursion of res_hostent_by_domain/4). You probably need to have both lowercased aliases and the original ones in the loop to create the correct(tm) hostent record at the end.

I also suspect that you need to fix the hostent_by_domain/3 function too since it looks up a CNAME, gets a lowercased from the cache and the compares it agains non-lowercased [Domain | Aliases].

If I remember this right the caching used for /etc/hosts lookup retains the case from the file so when you read an entry from the cache it will return the original casing from the /etc/hosts file. I understand your code stores lowercase in the cache but you return the original casing from the query. Correct? Which is the better behaviour?

Contributor

RaimoNiskanen commented Sep 10, 2015

I suspect that there is an irregularity in that returned aliases become lowercased (in the recursion of res_hostent_by_domain/4). You probably need to have both lowercased aliases and the original ones in the loop to create the correct(tm) hostent record at the end.

I also suspect that you need to fix the hostent_by_domain/3 function too since it looks up a CNAME, gets a lowercased from the cache and the compares it agains non-lowercased [Domain | Aliases].

If I remember this right the caching used for /etc/hosts lookup retains the case from the file so when you read an entry from the cache it will return the original casing from the /etc/hosts file. I understand your code stores lowercase in the cache but you return the original casing from the query. Correct? Which is the better behaviour?

@weiss

This comment has been minimized.

Show comment
Hide comment
@weiss

weiss Sep 10, 2015

Contributor

I suspect that there is an irregularity in that returned aliases become lowercased (in the recursion of res_hostent_by_domain/4).

That's true, but is there any value in having any upper-casing of the aliases retained from the original DNS response? I would've thought that's not worth any trouble, but I may well be overlooking something.

I also suspect that you need to fix the hostent_by_domain/3 function too since it looks up a CNAME, gets a lowercased from the cache and the compares it agains non-lowercased [Domain | Aliases].

Good point, thanks! I guess not only the CNAME lookup but also the lookup_type/2 call in that function needs to be fixed. I'll look into that.

If I remember this right the caching used for /etc/hosts lookup retains the case from the file so when you read an entry from the cache it will return the original casing from the /etc/hosts file. I understand your code stores lowercase in the cache but you return the original casing from the query. Correct?

Yes, that's right.

Which is the better behaviour?

I guess it might sometimes be convenient for the caller to have the casing retained from the query (when matching the result against the query, for example). Of course, the caller cannot safely assume either behaviour unless it's documented, so both ways should work. In short: I don't know.

Contributor

weiss commented Sep 10, 2015

I suspect that there is an irregularity in that returned aliases become lowercased (in the recursion of res_hostent_by_domain/4).

That's true, but is there any value in having any upper-casing of the aliases retained from the original DNS response? I would've thought that's not worth any trouble, but I may well be overlooking something.

I also suspect that you need to fix the hostent_by_domain/3 function too since it looks up a CNAME, gets a lowercased from the cache and the compares it agains non-lowercased [Domain | Aliases].

Good point, thanks! I guess not only the CNAME lookup but also the lookup_type/2 call in that function needs to be fixed. I'll look into that.

If I remember this right the caching used for /etc/hosts lookup retains the case from the file so when you read an entry from the cache it will return the original casing from the /etc/hosts file. I understand your code stores lowercase in the cache but you return the original casing from the query. Correct?

Yes, that's right.

Which is the better behaviour?

I guess it might sometimes be convenient for the caller to have the casing retained from the query (when matching the result against the query, for example). Of course, the caller cannot safely assume either behaviour unless it's documented, so both ways should work. In short: I don't know.

@RaimoNiskanen

This comment has been minimized.

Show comment
Hide comment
@RaimoNiskanen

RaimoNiskanen Sep 10, 2015

Contributor

That's true, but is there any value in having any upper-casing of the aliases retained from the
original DNS response? I would've thought that's not worth any trouble, but I may well be
overlooking something.

It just feels less obtrusive, and would not be that hard to fix. It would be consistent with what would happen if you yourself looked up the CNAME, got an uppercased answer and then looked it up.

Good point, thanks! I guess not only the CNAME lookup but also the lookup_type/2 call in that
function needs to be fixed. I'll look into that.

That might already be fixed since you changed do_lookup_rr/3.

Of course, the caller cannot safely assume either behaviour unless it's documented, so both ways
should work. In short: I don't know.

Neither do I. Retaining the original case feels logical. But keeping that original response would cost time when comparing or memory when storing, and returning what the caller entered feels more functional.

The in the future we should strive for making both caches behave the same...

Contributor

RaimoNiskanen commented Sep 10, 2015

That's true, but is there any value in having any upper-casing of the aliases retained from the
original DNS response? I would've thought that's not worth any trouble, but I may well be
overlooking something.

It just feels less obtrusive, and would not be that hard to fix. It would be consistent with what would happen if you yourself looked up the CNAME, got an uppercased answer and then looked it up.

Good point, thanks! I guess not only the CNAME lookup but also the lookup_type/2 call in that
function needs to be fixed. I'll look into that.

That might already be fixed since you changed do_lookup_rr/3.

Of course, the caller cannot safely assume either behaviour unless it's documented, so both ways
should work. In short: I don't know.

Neither do I. Retaining the original case feels logical. But keeping that original response would cost time when comparing or memory when storing, and returning what the caller entered feels more functional.

The in the future we should strive for making both caches behave the same...

@OTP-Maintainer

This comment has been minimized.

Show comment
Hide comment
@OTP-Maintainer

OTP-Maintainer Sep 24, 2015

Patch has passed first testings and has been assigned to be reviewed


I am a script, I am not human


Patch has passed first testings and has been assigned to be reviewed


I am a script, I am not human


@weiss

This comment has been minimized.

Show comment
Hide comment
@weiss

weiss Sep 24, 2015

Contributor

That's true, but is there any value in having any upper-casing of the aliases retained from the
original DNS response?

It just feels less obtrusive, and would not be that hard to fix. It would be consistent with what would happen if you yourself looked up the CNAME, got an uppercased answer and then looked it up.

Okay, I'll update the patch accordingly (later today).

I guess not only the CNAME lookup but also the lookup_type/2 call in that
function needs to be fixed. I'll look into that.

That might already be fixed since you changed do_lookup_rr/3.

You're completely right, of course.

Contributor

weiss commented Sep 24, 2015

That's true, but is there any value in having any upper-casing of the aliases retained from the
original DNS response?

It just feels less obtrusive, and would not be that hard to fix. It would be consistent with what would happen if you yourself looked up the CNAME, got an uppercased answer and then looked it up.

Okay, I'll update the patch accordingly (later today).

I guess not only the CNAME lookup but also the lookup_type/2 call in that
function needs to be fixed. I'll look into that.

That might already be fixed since you changed do_lookup_rr/3.

You're completely right, of course.

@weiss

This comment has been minimized.

Show comment
Hide comment
@weiss

weiss Sep 27, 2015

Contributor

I've now force-pushed an update that addresses the mentioned issues. The patch no longer lower-cases returned aliases, and the CNAME comparison within the hostent_by_domain/3 function is fixed.

Thanks a lot for the review.

Contributor

weiss commented Sep 27, 2015

I've now force-pushed an update that addresses the mentioned issues. The patch no longer lower-cases returned aliases, and the CNAME comparison within the hostent_by_domain/3 function is fixed.

Thanks a lot for the review.

@OTP-Maintainer

This comment has been minimized.

Show comment
Hide comment
@OTP-Maintainer

OTP-Maintainer Sep 29, 2015

Patch has passed first testings and has been assigned to be reviewed


I am a script, I am not human


Patch has passed first testings and has been assigned to be reviewed


I am a script, I am not human


@proxyles proxyles merged commit 68aac2e into erlang:maint Nov 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment