Merge branch 'raimo/kernel/inet_res-timeout-return/ERIERL-452/OTP-16414' into maint

* raimo/kernel/inet_res-timeout-return/ERIERL-452/OTP-16414:
  Tidy up timeout handling
  Keep intermediate error
  Test intermediate error

3 files changed, 241 insertions, 90 deletions

diff --git a/lib/kernel/src/inet_dns.erl b/lib/kernel/src/inet_dns.erl
index 6c98d2aab7..e03f124fe6 100644
--- a/lib/kernel/src/inet_dns.erl
+++ b/lib/kernel/src/inet_dns.erl
@@ -1,7 +1,7 @@
 %%
 %% %CopyrightBegin%
 %%
-%% Copyright Ericsson AB 1997-2017. All Rights Reserved.
+%% Copyright Ericsson AB 1997-2020. All Rights Reserved.
 %%
 %% Licensed under the Apache License, Version 2.0 (the "License");
 %% you may not use this file except in compliance with the License.
@@ -109,8 +109,8 @@ lists_member(H, [H|_]) -> true;
 lists_member(H, [_|T]) -> lists_member(H, T).
 
 
-
--define(DECODE_ERROR, fmt). % must match a clause in inet_res:query_nss_e?dns
+%% must match a clause in inet_res:query_nss_e?dns
+-define(DECODE_ERROR, formerr).
 
 %%
 %% Decode a dns buffer.
diff --git a/lib/kernel/src/inet_res.erl b/lib/kernel/src/inet_res.erl
index 6454802b04..7886ef83ac 100644
--- a/lib/kernel/src/inet_res.erl
+++ b/lib/kernel/src/inet_res.erl
@@ -1,7 +1,7 @@
 %%
 %% %CopyrightBegin%
 %%
-%% Copyright Ericsson AB 1997-2018. All Rights Reserved.
+%% Copyright Ericsson AB 1997-2020. All Rights Reserved.
 %%
 %% Licensed under the Apache License, Version 2.0 (the "License");
 %% you may not use this file except in compliance with the License.
@@ -691,21 +691,22 @@ udp_send(#sock{inet=I}, {A,B,C,D}=IP, Port, Buffer)
     gen_udp:send(I, IP, Port, Buffer).
 
 udp_recv(#sock{inet6=I}, {A,B,C,D,E,F,G,H}=IP, Port, Timeout, Decode)
-  when ?ip6(A,B,C,D,E,F,G,H), ?port(Port) ->
-    do_udp_recv(I, IP, Port, Timeout, Decode, time_now(), Timeout);
+  when ?ip6(A,B,C,D,E,F,G,H), ?port(Port), 0 =< Timeout ->
+    do_udp_recv(I, IP, Port, Timeout, Decode, deadline(Timeout), Timeout);
 udp_recv(#sock{inet=I}, {A,B,C,D}=IP, Port, Timeout, Decode)
-  when ?ip(A,B,C,D), ?port(Port) ->
-    do_udp_recv(I, IP, Port, Timeout, Decode, time_now(), Timeout).
+  when ?ip(A,B,C,D), ?port(Port), 0 =< Timeout ->
+    do_udp_recv(I, IP, Port, Timeout, Decode, deadline(Timeout), Timeout).
 
-do_udp_recv(_I, _IP, _Port, 0, _Decode, _Start, _T) ->
+do_udp_recv(_I, _IP, _Port, 0, _Decode, _Deadline, PollCnt)
+  when PollCnt =< 0 ->
     timeout;
-do_udp_recv(I, IP, Port, Timeout, Decode, Start, T) ->
-    case gen_udp:recv(I, 0, T) of
+do_udp_recv(I, IP, Port, Timeout, Decode, Deadline, PollCnt) ->
+    case gen_udp:recv(I, 0, Timeout) of
 	{ok,Reply} ->
 	    case Decode(Reply) of
-		false when T =:= 0 ->
+		false when Timeout =:= 0 ->
 		    %% This is a compromize between the hard way i.e
-		    %% in the clause below if NewT becomes 0 bailout
+		    %% in the clause below if Timeout becomes 0 bailout
 		    %% immediately and risk that the right reply lies
 		    %% ahead after some bad id replies, and the
 		    %% forgiving way i.e go on with Timeout 0 until
@@ -713,15 +714,12 @@ do_udp_recv(I, IP, Port, Timeout, Decode, Start, T) ->
 		    %% which opens for a DOS attack by a malicious
 		    %% DNS server flooding with bad id replies causing
 		    %% an infinite loop here.
-		    %%
-		    %% Timeout is used as a sanity limit counter
-		    %% just to put an end to the loop.
-		    NewTimeout = erlang:max(0, Timeout - 50),
-		    do_udp_recv(I, IP, Port, NewTimeout, Decode, Start, T);
+                    %%
+		    do_udp_recv(
+                      I, IP, Port, Timeout, Decode, Deadline, PollCnt-50);
 		false ->
-		    Now = time_now(),
-		    NewT = erlang:max(0, Timeout - now_ms(Now, Start)),
-		    do_udp_recv(I, IP, Port, Timeout, Decode, Start, NewT);
+                    T = timeout(Deadline),
+		    do_udp_recv(I, IP, Port, T, Decode, Deadline, PollCnt);
 		Result ->
 		    Result
 	    end;
@@ -758,71 +756,122 @@ udp_close(#sock{inet=I,inet6=I6}) ->
 %% And that is what the code seems to do, now fixed, hopefully...
 
 do_query(_Q, [], _Timer) ->
+    %% We have no name server to ask, so say nxdomain
     {error,nxdomain};
 do_query(#q{options=#options{retry=Retry}}=Q, NSs, Timer) ->
-    query_retries(Q, NSs, Timer, Retry, 0, #sock{}).
-
-query_retries(_Q, _NSs, _Timer, Retry, Retry, S) ->
-    udp_close(S),
-    {error,timeout};
-query_retries(_Q, [], _Timer, _Retry, _I, S) ->
-    udp_close(S),
-    {error,timeout};
-query_retries(Q, NSs, Timer, Retry, I, S0) ->
-    case query_nss(Q, NSs, Timer, Retry, I, S0, []) of
-	{S,{noanswer,ErrNSs}} -> %% remove unreachable nameservers
-	    query_retries(Q, NSs--ErrNSs, Timer, Retry, I+1, S);
-	{S,Result} ->
-	    udp_close(S),
-	    Result
-    end.
+    %% We have at least one name server,
+    %% so a failure will be a timeout,
+    %% unless a name server says otherwise
+    Reason = timeout,
+    query_retries(Q, NSs, Timer, Retry, 0, #sock{}, Reason).
 
-query_nss(_Q, [], _Timer, _Retry, _I, S, ErrNSs) ->
-    {S,{noanswer,ErrNSs}};
-query_nss(#q{edns=undefined}=Q, NSs, Timer, Retry, I, S, ErrNSs) ->
-    query_nss_dns(Q, NSs, Timer, Retry, I, S, ErrNSs);
-query_nss(Q, NSs, Timer, Retry, I, S, ErrNSs) ->
-    query_nss_edns(Q, NSs, Timer, Retry, I, S, ErrNSs).
+%% Loop until out of name servers or retries
+%%
+query_retries(_Q, _NSs, _Timer, Retry, Retry, S, Reason) ->
+    query_retries_error(S, Reason);
+query_retries(_Q, [], _Timer, _Retry, _I, S, Reason) ->
+    query_retries_error(S, Reason);
+query_retries(Q, NSs, Timer, Retry, I, S_0, Reason) ->
+    query_nss(Q, NSs, Timer, Retry, I, S_0, Reason, NSs).
+
+%% Loop for all name servers, for each:
+%%     If EDNS is enabled, try that first,
+%%     and for selected failures fall back to plain DNS.
+%%
+query_nss(Q, NSs, Timer, Retry, I, S, Reason, []) ->
+    %% End of name servers list, do a new retry
+    query_retries(Q, NSs, Timer, Retry, I+1, S, Reason);
+query_nss(#q{edns = undefined}=Q, NSs, Timer, Retry, I, S, Reason, TryNSs) ->
+    query_nss_dns(Q, NSs, Timer, Retry, I, S, Reason, TryNSs);
+query_nss(Q, NSs, Timer, Retry, I, S, Reason, TryNSs) ->
+    query_nss_edns(Q, NSs, Timer, Retry, I, S, Reason, TryNSs).
 
 query_nss_edns(
-  #q{options=#options{udp_payload_size=PSz}=Options,edns={Id,Buffer}}=Q,
-  [{IP,Port}=NS|NSs]=NSs0, Timer, Retry, I, S0, ErrNSs) ->
-    {S,Res}=Reply =
-	query_ns(S0, Id, Buffer, IP, Port, Timer, Retry, I, Options, PSz),
-    case Res of
-	timeout -> {S,{error,timeout}}; % Bailout timeout
-	{ok,_} -> Reply;
-	{error,{nxdomain,_}} -> Reply;
-	{error,{E,_}} when E =:= qfmterror; E =:= notimp; E =:= servfail;
-			   E =:= badvers ->
-	    query_nss_dns(Q, NSs0, Timer, Retry, I, S, ErrNSs);
-	{error,E} when E =:= fmt; E =:= enetunreach; E =:= econnrefused ->
-	    query_nss(Q, NSs, Timer, Retry, I, S, [NS|ErrNSs]);
-	_Error ->
-	    query_nss(Q, NSs, Timer, Retry, I, S, ErrNSs)
+  #q{options =
+         #options{
+            udp_payload_size = PSz}=Options,
+     edns = {Id,Buffer}}=Q,
+  NSs, Timer, Retry, I, S_0, Reason, [{IP,Port}=NS|TryNSs]=TryNSs_0) ->
+    %%
+    {S,Result} =
+	query_ns(
+          S_0, Id, Buffer, IP, Port, Timer, Retry, I, Options, PSz),
+    case Result of
+	{error,{E,_}}
+          when E =:= qfmterror;
+               E =:= notimp;
+               E =:= servfail;
+               E =:= badvers ->
+            %% The server did not like that,
+            %% ignore that error and try plain DNS
+	    query_nss_dns(
+              Q, NSs, Timer, Retry, I, S, Reason, TryNSs_0);
+        _ ->
+	    query_nss_result(
+              Q, NSs, Timer, Retry, I, S, Reason, TryNSs, NS, Result)
     end.
 
 query_nss_dns(
-  #q{dns=Qdns}=Q0,
-  [{IP,Port}=NS|NSs], Timer, Retry, I, S0, ErrNSs) ->
-    #q{options=Options,dns={Id,Buffer}}=Q =
+  #q{dns = Qdns}=Q_0,
+  NSs, Timer, Retry, I, S_0, Reason, [{IP,Port}=NS|TryNSs]) ->
+    %%
+    #q{options = Options,
+       dns = {Id,Buffer}}=Q =
 	if
-	    is_function(Qdns, 0) -> Q0#q{dns=Qdns()};
-	    true -> Q0
+	    is_function(Qdns, 0) -> Q_0#q{dns=Qdns()};
+	    true -> Q_0
 	end,
-    {S,Res}=Reply =
+    {S,Result} =
 	query_ns(
-	  S0, Id, Buffer, IP, Port, Timer, Retry, I, Options, ?PACKETSZ),
-    case Res of
-	timeout -> {S,{error,timeout}}; % Bailout timeout
-	{ok,_} -> Reply;
-	{error,{E,_}} when E =:= nxdomain; E =:= qfmterror -> Reply;
-	{error,E} when E =:= fmt; E =:= enetunreach; E =:= econnrefused ->
-	    query_nss(Q, NSs, Timer, Retry, I, S, [NS|ErrNSs]);
-	_Error ->
-	    query_nss(Q, NSs, Timer, Retry, I, S, ErrNSs)
+	  S_0, Id, Buffer, IP, Port, Timer, Retry, I, Options, ?PACKETSZ),
+    query_nss_result(
+      Q, NSs, Timer, Retry, I, S, Reason, TryNSs, NS, Result).
+
+query_nss_result(Q, NSs, Timer, Retry, I, S, Reason, TryNSs, NS, Result) ->
+    case Result of
+	{ok,_} ->
+            _ = udp_close(S),
+            Result;
+	timeout -> % Out of total time timeout
+            query_retries_error(S, Reason); % The best reason we have
+	{error,timeout} -> % Query timeout
+            %% Try next server, may retry this server later
+	    query_nss(Q, NSs, Timer, Retry, I, S, Reason, TryNSs);
+	{error,{nxdomain,_}=NewReason} ->
+            query_retries_error(S, NewReason); % Definite answer
+	{error,{E,_}=NewReason}
+          when E =:= qfmterror;
+               E =:= notimp;
+               E =:= refused;
+               E =:= badvers ->
+            %% The server did not like that.
+            %% Remove this server from retry list since
+            %% it will not answer differently on the next retry.
+            NewNSs = lists:delete(NS, NSs),
+	    query_nss(Q, NewNSs, Timer, Retry, I, S, NewReason, TryNSs);
+	{error,E=NewReason}
+          when E =:= formerr;
+               E =:= enetunreach;
+               E =:= econnrefused ->
+            %% Could not decode answer, or network problem.
+            %% Remove this server from retry list.
+            NewNSs = lists:delete(NS, NSs),
+	    query_nss(Q, NewNSs, Timer, Retry, I, S, NewReason, TryNSs);
+	{error,NewReason} ->
+            %% Try next server, may retry this server later
+	    query_nss(Q, NSs, Timer, Retry, I, S, NewReason, TryNSs)
     end.
 
+query_retries_error(S, Reason) ->
+    _ = udp_close(S),
+    case Reason of
+        {nxdomain, _} ->
+            {error, nxdomain};
+        _ ->
+            {error, Reason}
+    end.
+
+
 query_ns(S0, Id, Buffer, IP, Port, Timer, Retry, I,
 	 #options{timeout=Tm,usevc=UseVC,verbose=Verbose},
 	 PSz) ->
@@ -1035,10 +1084,11 @@ dns_msg(Msg) ->
 	    {Type,dns_msg(Fields)}
     end.
 
--compile({inline, [now_ms/2]}).
-now_ms(Int1, Int0) ->
-    Int1 - Int0.
-
--compile({inline, [time_now/0]}).
-time_now() ->
-	erlang:monotonic_time(1000).
+-compile({inline, [deadline/1, timeout/1]}).
+deadline(Timeout) -> % When is the deadline? [ms]
+    erlang:monotonic_time(1000) + Timeout.
+timeout(Deadline) -> % How long to deadline? [ms] >= 0
+    case Deadline - erlang:monotonic_time(1000) of
+        Timeout when 0 =< Timeout -> Timeout;
+        _ -> 0
+    end.
diff --git a/lib/kernel/test/inet_res_SUITE.erl b/lib/kernel/test/inet_res_SUITE.erl
index 6b545fa414..8b3f1aa2a9 100644
--- a/lib/kernel/test/inet_res_SUITE.erl
+++ b/lib/kernel/test/inet_res_SUITE.erl
@@ -1,7 +1,7 @@
 %%
 %% %CopyrightBegin%
 %%
-%% Copyright Ericsson AB 2009-2018. All Rights Reserved.
+%% Copyright Ericsson AB 2009-2020. All Rights Reserved.
 %%
 %% Licensed under the Apache License, Version 2.0 (the "License");
 %% you may not use this file except in compliance with the License.
@@ -28,7 +28,7 @@
 	 init_per_group/2,end_per_group/2,
 	 init_per_testcase/2, end_per_testcase/2]).
 -export([basic/1, resolve/1, edns0/1, txt_record/1, files_monitor/1,
-	 last_ms_answer/1]).
+	 last_ms_answer/1, intermediate_error/1]).
 -export([
 	 gethostbyaddr/0, gethostbyaddr/1,
 	 gethostbyaddr_v6/0, gethostbyaddr_v6/1,
@@ -64,7 +64,7 @@ suite() ->
 
 all() -> 
     [basic, resolve, edns0, txt_record, files_monitor,
-     last_ms_answer,
+     last_ms_answer, intermediate_error,
      gethostbyaddr, gethostbyaddr_v6, gethostbyname,
      gethostbyname_v6, getaddr, getaddr_v6, ipv4_to_ipv6,
      host_and_addr].
@@ -91,6 +91,9 @@ zone_dir(TC) ->
 	edns0 -> otptest;
 	files_monitor -> otptest;
 	last_ms_answer -> otptest;
+        intermediate_error ->
+            {internal,
+             #{rcode => ?REFUSED}};
 	_ -> undefined
     end.
 
@@ -106,6 +109,9 @@ init_per_testcase(Func, Config) ->
 		    inet_db:ins_alt_ns(IP, Port);
 		_ -> ok
 	    end,
+            %% dbg:tracer(),
+            %% dbg:p(all, c),
+            %% dbg:tpl(inet_res, query_nss_res, cx),
 	    [{nameserver,NsSpec},{res_lookup,Lookup}|Config]
     catch
 	SkipReason ->
@@ -120,6 +126,7 @@ end_per_testcase(_Func, Config) ->
 	    inet_db:del_alt_ns(IP, Port);
 	_ -> ok
     end,
+    %% dbg:stop(),
     ns_end(NsSpec, proplists:get_value(priv_dir, Config)).
 
 %% %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
@@ -130,15 +137,18 @@ ns(Config) ->
     NS.
 
 ns_init(ZoneDir, PrivDir, DataDir) ->
-    case os:type() of
-	{unix,_} when ZoneDir =:= undefined -> undefined;
-	{unix,_} ->
+    case {os:type(),ZoneDir} of
+        {_,{internal,ServerSpec}} ->
+            ns_start_internal(ServerSpec);
+	{{unix,_},undefined} ->
+            undefined;
+	{{unix,_},otptest} ->
 	    PortNum = case {os:type(),os:version()} of
 			  {{unix,solaris},{M,V,_}} when M =< 5, V < 10 ->
 			      11895 + rand:uniform(100);
 			  _ ->
-			      {ok,S} = gen_udp:open(0, [{reuseaddr,true}]),
-			      {ok,PNum} = inet:port(S),
+			      S = ok(gen_udp:open(0, [{reuseaddr,true}])),
+			      PNum = ok(inet:port(S)),
 			      gen_udp:close(S),
 			      PNum
 		      end,
@@ -170,12 +180,43 @@ ns_start(ZoneDir, PrivDir, NS, P) ->
 	    ns_start(ZoneDir, PrivDir, NS, P)
     end.
 
+ns_start_internal(ServerSpec) ->
+    Parent = self(),
+    Tag = make_ref(),
+    {P,Mref} =
+        spawn_monitor(
+          fun () ->
+                  _ = process_flag(trap_exit, true),
+                  IP = {127,0,0,1},
+                  SocketOpts = [{ip,IP},binary,{active,once}],
+                  S = ok(gen_udp:open(0, SocketOpts)),
+                  Port = ok(inet:port(S)),
+                  ParentMref = monitor(process, Parent),
+                  Parent ! {Tag,{IP,Port},self()},
+                  ns_internal(ServerSpec, ParentMref, Tag, S)
+          end),
+    receive
+        {Tag,_NS,P} = NsSpec ->
+            demonitor(Mref, [flush]),
+            NsSpec;
+        {'DOWN',Mref,_,_,Reason} ->
+            exit({ns_start_internal,Reason})
+    end.
+
 ns_end(undefined, _PrivDir) -> undefined;
-ns_end({ZoneDir,_NS,P}, PrivDir) ->
+ns_end({ZoneDir,_NS,P}, PrivDir) when is_port(P) ->
     port_command(P, ["quit",io_lib:nl()]),
     ns_stop(P),
     ns_printlog(filename:join([PrivDir,ZoneDir,"named.log"])),
-    ok.
+    ok;
+ns_end({Tag,_NS,P}, _PrivDir) when is_pid(P) ->
+    Mref = erlang:monitor(process, P),
+    P ! Tag,
+    receive
+        {'DOWN',Mref,_,_,Reason} ->
+            Reason = normal,
+            ok
+    end.
 
 ns_stop(P) ->
     case ns_collect(P) of
@@ -209,6 +250,44 @@ ns_printlog(Fname) ->
     end.
 
 %% %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
+%% Internal name server
+
+ns_internal(ServerSpec, Mref, Tag, S) ->
+    receive
+        {'DOWN',Mref,_,_,Reason} ->
+            exit(Reason);
+        Tag ->
+            ok;
+        {udp,S,IP,Port,Data} ->
+            Req = ok(inet_dns:decode(Data)),
+            Resp = ns_internal(ServerSpec, Req),
+            RespData = inet_dns:encode(Resp),
+            _ = ok(gen_udp:send(S, IP, Port, RespData)),
+            _ = ok(inet:setopts(S, [{active,once}])),
+            ns_internal(ServerSpec, Mref, Tag, S)
+    end.
+
+ns_internal(#{rcode := Rcode}, Req) ->
+    Hdr = inet_dns:msg(Req, header),
+    Opcode = inet_dns:header(Hdr, opcode),
+    Id = inet_dns:header(Hdr, id),
+    Rd = inet_dns:header(Hdr, rd),
+    %%
+    Qdlist = inet_dns:msg(Req, qdlist),
+    inet_dns:make_msg(
+       [{header,
+         inet_dns:make_header(
+           [{id,Id},
+            {qr,true},
+            {opcode,Opcode},
+            {aa,true},
+            {tc,false},
+            {rd,Rd},
+            {ra,false},
+            {rcode,Rcode}])},
+         {qdlist,Qdlist}]).
+
+%% %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 %% Behaviour modifying nameserver proxy
 
 proxy_start(TC, {NS,P}) ->
@@ -672,6 +751,23 @@ last_ms_answer(Config) when is_list(Config) ->
     ok.
 
 %% %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
+%% First name server answers ?REFUSED, second does not answer.
+%% Check that we get the error code from the first server.
+
+intermediate_error(Config) when is_list(Config) ->
+    NS = ns(Config),
+    Name = "ns.otptest",
+    IP = {127,0,0,1},
+    %% A "name server" that does not respond
+    S = ok(gen_udp:open(0, [{ip,IP},{active,false}])),
+    Port = ok(inet:port(S)),
+    NSs = [NS,{IP,Port}],
+    {error,{refused,_}} =
+        inet_res:resolve(Name, in, a, [{nameservers,NSs},verbose], 500),
+    _ = gen_udp:close(S),
+    ok.
+
+%% %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 %% Compatibility tests. Call the inet_SUITE tests, but with
 %% lookup = [file,dns] instead of [native]
 
@@ -729,3 +825,8 @@ tolower([C|Cs]) when is_integer(C) ->
     end;
 tolower([]) ->
     [].
+
+-compile({inline,[ok/1]}).
+ok(ok) -> ok;
+ok({ok,X}) -> X;
+ok({error,Reason}) -> error(Reason).