From 5228b6e5e3906a7a26e2e730b9f0213844e967a8 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sat, 21 Mar 2015 10:56:28 +0100 Subject: Be lenient with errors in incoming DPR To avoid having the peer interpret the error as meaning the connection shouldn't be closed, which probably does more harm than ignoring syntactic errors in the DPR. Note that RFC 6733 says this about incoming DPR, in 5.4 Disconnecting Peer Connections: Upon receipt of the message, the Disconnect-Peer-Answer message is returned, which SHOULD contain an error if messages have recently been forwarded, and are likely in flight, which would otherwise cause a race condition. The race here is presumably between answers to forwarded requests and the outgoing DPA, but we have no handling for this: whether or not there are pending answers is irrelevant to how DPR is answered. It's questionable that a peer should be able to prevent disconnection in any case: it has to be the node sending DPR that decides if it's approriate, and the peer should take it as an indication of what's coming. Incoming DPA is already treated leniently: the only error that's not ignored is mismatching End-to-End and Hop-by-Hop Identifiers, since there's no distinguishing an erroneous value from an unsolicited DPA. This mismatch could also be ignored, which is the case for DWA for example, but this problem is already dealt with by dpa_timeout, which causes a connection to be closed even when the expected DPA isn't received. --- lib/diameter/src/base/diameter_peer_fsm.erl | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) (limited to 'lib/diameter/src/base') diff --git a/lib/diameter/src/base/diameter_peer_fsm.erl b/lib/diameter/src/base/diameter_peer_fsm.erl index 9ff6845ab7..8bfdb3ae39 100644 --- a/lib/diameter/src/base/diameter_peer_fsm.erl +++ b/lib/diameter/src/base/diameter_peer_fsm.erl @@ -642,7 +642,9 @@ rcv('DPA' = N, diameter_peer:close(TPid), {stop, N}; -%% Ignore anything else, an unsolicited DPA in particular. +%% Ignore anything else, an unsolicited DPA in particular. Note that +%% dpa_timeout deals with the case in which the peer sends the wrong +%% identifiers in DPA. rcv(N, #diameter_packet{header = H}, _) when N == 'CER'; N == 'CEA'; @@ -820,7 +822,7 @@ build_answer(Type, errors = Es} = Pkt, S) -> - {RC, FailedAVP} = result_code(H, Es), + {RC, FailedAVP} = result_code(Type, H, Es), {answer(Type, RC, FailedAVP, S), post(Type, RC, Pkt, S)}. inband_security([]) -> @@ -890,6 +892,19 @@ set(['answer-message' | _] = Ans, FailedAvp) -> set([_|_] = Ans, FailedAvp) -> Ans ++ FailedAvp. +%% result_code/3 + +%% Be lenient with errors in DPR since there's no reason to be +%% otherwise. Rejecting may cause the peer to missinterpret the error +%% as meaning that the connection should not be closed, which may well +%% lead to more problems than any errors in the DPR. + +result_code('DPR', _, _) -> + {2001, []}; + +result_code('CER', H, Es) -> + result_code(H, Es). + %% result_code/2 result_code(#diameter_header{is_error = true}, _) -> -- cgit v1.2.3 From 9f496aaba6e8a63180eca3a77d01205ca0fcbff7 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sat, 21 Mar 2015 10:11:51 +0100 Subject: Add transport_opt() dpr_timeout To cause a peer connection to be closed following an outgoing DPA, in case the peer fails to do so. It is the recipient of DPA that should close the connection according to RFC 6733. --- lib/diameter/src/base/diameter.erl | 1 + lib/diameter/src/base/diameter_config.erl | 1 + lib/diameter/src/base/diameter_peer_fsm.erl | 46 ++++++++++++++++++++++------- 3 files changed, 37 insertions(+), 11 deletions(-) (limited to 'lib/diameter/src/base') diff --git a/lib/diameter/src/base/diameter.erl b/lib/diameter/src/base/diameter.erl index a45d84f95b..da2695e1be 100644 --- a/lib/diameter/src/base/diameter.erl +++ b/lib/diameter/src/base/diameter.erl @@ -343,6 +343,7 @@ call(SvcName, App, Message) -> | {capabilities_cb, evaluable()} | {capx_timeout, 'Unsigned32'()} | {disconnect_cb, evaluable()} + | {dpr_timeout, 'Unsigned32'()} | {dpa_timeout, 'Unsigned32'()} | {length_errors, exit | handle | discard} | {connect_timer, 'Unsigned32'()} diff --git a/lib/diameter/src/base/diameter_config.erl b/lib/diameter/src/base/diameter_config.erl index aa4d6e5a20..a89831aa5e 100644 --- a/lib/diameter/src/base/diameter_config.erl +++ b/lib/diameter/src/base/diameter_config.erl @@ -534,6 +534,7 @@ opt({capabilities, Os}) -> opt({K, Tmo}) when K == capx_timeout; + K == dpr_timeout; K == dpa_timeout -> ?IS_UINT32(Tmo); diff --git a/lib/diameter/src/base/diameter_peer_fsm.erl b/lib/diameter/src/base/diameter_peer_fsm.erl index 8bfdb3ae39..8d67f0aa90 100644 --- a/lib/diameter/src/base/diameter_peer_fsm.erl +++ b/lib/diameter/src/base/diameter_peer_fsm.erl @@ -63,7 +63,8 @@ %% Keys in process dictionary. -define(CB_KEY, cb). %% capabilities callback -define(DPR_KEY, dpr). %% disconnect callback --define(DPA_KEY, dpa). %% timeout for DPA reception +-define(DPA_KEY, dpa). %% timeout for incoming DPA, or shutdown after + %% outgoing DPA -define(REF_KEY, ref). %% transport_ref() -define(Q_KEY, q). %% transport start queue -define(START_KEY, start). %% start of connected transport @@ -83,18 +84,26 @@ N == ?GOAWAY; N == goaway; N == ?BUSY; N == busy). -%% RFC 3588: +%% RFC 6733: %% %% Timeout An application-defined timer has expired while waiting %% for some event. %% --define(EVENT_TIMEOUT, 10000). + %% Default timeout for reception of CER/CEA. +-define(CAPX_TIMEOUT, 10000). -%% Default timeout for DPA in response to DPR. A bit short but the -%% timeout used to be hardcoded. (So it could be worse.) +%% Default timeout for DPA to be received in response to an outgoing +%% DPR. A bit short but the timeout used to be hardcoded. (So it could +%% be worse.) -define(DPA_TIMEOUT, 1000). +%% Default timeout for the connection to be closed by the peer +%% following an outgoing DPA in response to an incoming DPR. It's the +%% recipient of DPA that should close the connection according to the +%% RFC. +-define(DPR_TIMEOUT, 5000). + -type uint32() :: diameter:'Unsigned32'(). -record(state, @@ -189,9 +198,10 @@ i({Ack, WPid, {M, Ref} = T, Opts, {Mask, Nodes, Dict0, Svc}}) -> putr(?REF_KEY, Ref), putr(?SEQUENCE_KEY, Mask), putr(?RESTRICT_KEY, Nodes), - putr(?DPA_KEY, proplists:get_value(dpa_timeout, Opts, ?DPA_TIMEOUT)), + putr(?DPA_KEY, {proplists:get_value(dpr_timeout, Opts, ?DPR_TIMEOUT), + proplists:get_value(dpa_timeout, Opts, ?DPA_TIMEOUT)}), - Tmo = proplists:get_value(capx_timeout, Opts, ?EVENT_TIMEOUT), + Tmo = proplists:get_value(capx_timeout, Opts, ?CAPX_TIMEOUT), OnLengthErr = proplists:get_value(length_errors, Opts, exit), {TPid, Addrs} = start_transport(T, Rest, Svc), @@ -416,7 +426,8 @@ transition({shutdown, Pid, Reason}, #state{parent = Pid, dpr = false} = S) -> transition({shutdown, Pid, _}, #state{parent = Pid}) -> ok; -%% DPA reception has timed out. +%% DPA reception has timed out, or peer has not closed the connection +%% as a result of outgoing DPA. transition(dpa_timeout, _) -> stop; @@ -840,7 +851,7 @@ cea(CEA, RC, Dict0) -> post('CER' = T, RC, Pkt, S) -> {T, caps(S), {RC, Pkt}}; post('DPR', _, _, #state{parent = Pid}) -> - [fun(S) -> inform_dpr(Pid), S end]. + [fun(S) -> dpr_timer(), inform_dpr(Pid), S end]. inform_dpr(Pid) -> Pid ! {'DPR', self()}. %% tell watchdog to die with us @@ -1247,11 +1258,24 @@ dpa_timer(Tmo) -> dpa_timeout() -> dpa_timeout(getr(?DPA_KEY)). -dpa_timeout(undefined) -> +dpa_timeout({_, Tmo}) -> + Tmo; +dpa_timeout(undefined) -> %% set in old code ?DPA_TIMEOUT; -dpa_timeout(Tmo) -> +dpa_timeout(Tmo) -> %% ditto Tmo. +dpr_timer() -> + dpa_timer(dpr_timeout()). + +dpr_timeout() -> + dpr_timeout(getr(?DPA_KEY)). + +dpr_timeout({Tmo, _}) -> + Tmo; +dpr_timeout(_) -> %% set in old code + ?DPR_TIMEOUT. + %% register_everywhere/1 %% %% Register a term and ensure it's not registered elsewhere. Note that -- cgit v1.2.3 From e541c3d17c7b8295201cd7d72e876c1c67d0fc50 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sat, 21 Mar 2015 12:07:32 +0100 Subject: Discard incoming/outgoing requests after incoming DPR With the same motivation as in commits 5bd2d72 and b1fd629. As in the latter, incoming DPR is the only exception. --- lib/diameter/src/base/diameter_peer_fsm.erl | 38 +++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 10 deletions(-) (limited to 'lib/diameter/src/base') diff --git a/lib/diameter/src/base/diameter_peer_fsm.erl b/lib/diameter/src/base/diameter_peer_fsm.erl index 8d67f0aa90..4ad4c346f4 100644 --- a/lib/diameter/src/base/diameter_peer_fsm.erl +++ b/lib/diameter/src/base/diameter_peer_fsm.erl @@ -117,9 +117,14 @@ transport :: pid(), %% transport process dictionary :: module(), %% common dictionary service :: #diameter_service{}, - dpr = false :: false | {uint32(), uint32()} %% set in old code - | {boolean(), uint32(), uint32()}, - %% | hop by hop and end to end identifiers + dpr = false :: false + | true %% DPR received, DPA sent + | {uint32(), uint32()} %% set in old code + | {boolean(), uint32(), uint32()}, + %% hop by hop and end to end identifiers in + %% outgoing DPR; boolean says whether or not + %% the request was sent explicitly with + %% diameter:call/4. length_errors :: exit | handle | discard}). %% There are non-3588 states possible as a consequence of 5.6.1 of the @@ -550,13 +555,19 @@ recv(Bin, S) -> %% recv1/3 -%% Incoming request after DPR has been sent: discard. Don't discard -%% DPR, so both ends don't do so when sending simultaneously. +%% Incoming request after outgoing DPR: discard. Don't discard DPR, so +%% both ends don't do so when sending simultaneously. recv1(Name, #diameter_packet{header = #diameter_header{is_request = true} = H}, #state{dpr = {_,_,_}}) when Name /= 'DPR' -> - invalid(false, recv_after_dpr, H); + invalid(false, recv_after_outgoing_dpr, H); + +%% Incoming request after incoming DPR: discard. +recv1(_, + #diameter_packet{header = #diameter_header{is_request = true} = H}, + #state{dpr = true}) -> + invalid(false, recv_after_incoming_dpr, H); %% DPA with identifier mismatch, or in response to a DPR initiated by %% the service. @@ -707,8 +718,10 @@ outgoing(#diameter_packet{header = #diameter_header{application_id = 0, if T == false -> inform_dpr(Pid), send_dpr(true, Pkt, dpa_timeout(), S); + T == true -> + invalid(false, dpr_after_dpa, H); %% DPA sent: discard true -> - invalid(false, dpr_after_dpr, H) %% already sent: discard + invalid(false, dpr_after_dpr, H) %% DPR sent: discard end; %% Explict CER or DWR: discard. These are sent by us. @@ -851,7 +864,12 @@ cea(CEA, RC, Dict0) -> post('CER' = T, RC, Pkt, S) -> {T, caps(S), {RC, Pkt}}; post('DPR', _, _, #state{parent = Pid}) -> - [fun(S) -> dpr_timer(), inform_dpr(Pid), S end]. + [fun(S) -> dpr_timer(), inform_dpr(Pid), dpr(S) end]. + +dpr(#state{dpr = false} = S) -> %% not awaiting DPA + S#state{dpr = true}; %% DPR received +dpr(S) -> %% DPR already sent or received + S. inform_dpr(Pid) -> Pid ! {'DPR', self()}. %% tell watchdog to die with us @@ -1144,7 +1162,7 @@ close(Reason) -> %% dpr/2 %% -%% The RFC isn't clear on whether DPR should be send in a non-Open +%% The RFC isn't clear on whether DPR should be sent in a non-Open %% state. The Peer State Machine transitions it documents aren't %% exhaustive (no Stop in Wait-I-CEA for example) so assume it's up to %% the implementation and transition to Closed (ie. die) if we haven't @@ -1160,7 +1178,7 @@ dpr(Reason, #state{state = 'Open', Peer = {self(), Caps}, dpr(CBs, [Reason, Ref, Peer], S); -%% Connection is open, DPR already sent. +%% Connection is open, DPR already sent or received. dpr(_, #state{state = 'Open'}) -> ok; -- cgit v1.2.3