From 9321b4b3aa332f140268ef8f3251f6314a4984fe Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Mon, 23 Mar 2015 07:57:26 +0100 Subject: Fix ordering of AVPs in relayed messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 6.1.9 of RFC 6733 states this: A relay or proxy agent MUST append a Route-Record AVP to all requests forwarded. The AVP was inserted as the head of the AVP list, not appended, since the entire AVP list was reversed relative to the received order. Thanks to Andrzej Trawiński. --- lib/diameter/src/base/diameter_traffic.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/diameter/src/base/diameter_traffic.erl') diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index 3b62afca47..3717e43e4a 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -592,7 +592,7 @@ resend(false, Route = #diameter_avp{data = {Dict0, 'Route-Record', OH}}, Seq = diameter_session:sequence(Mask), Hdr = Hdr0#diameter_header{hop_by_hop_id = Seq}, - Msg = [Hdr, Route | Avps], + Msg = [Hdr, Route | Avps], %% reordered at encode resend(send_request(SvcName, App, Msg, Opts), Caps, Dict0, Pkt). %% The incoming request is relayed with the addition of a %% Route-Record. Note the requirement on the return from call/4 below, -- cgit v1.2.3 From 75ee72b4e1d288b1d96194d0e352eb0c73db4a6f Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 12 Feb 2015 11:18:52 +0100 Subject: Strip potentially large terms when sending outgoing Diameter messages Both incoming and outgoing Diameter messages pass through two or three processes, depending on whether they're incoming or outgoing: the transport process and corresponding peer_fsm process and (for incoming) watchdog processes. Since terms other than binary are copied when passing process boundaries, large terms lead to copying that can be problematic, if frequent enough. Since only the bin and transport_data fields of a diameter_packet record are needed by the transport process, discard others when sending outgoing messages. Strictly speaking, the statement that only the aforementioned fields are needed by the transport process depends on the transport process. It's true of those implemented by diameter (in diameter_tcp and diameter_sctp), but an implementation that makes use of other fields is assuming more than the documentation in diameter_transport(3) promises. --- lib/diameter/src/base/diameter_traffic.erl | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) (limited to 'lib/diameter/src/base/diameter_traffic.erl') diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index 0b503338a6..18c1965f77 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2013-2014. All Rights Reserved. +%% Copyright Ericsson AB 2013-2015. All Rights Reserved. %% %% The contents of this file are subject to the Erlang Public License, %% Version 1.1, (the "License"); you may not use this file except in @@ -1679,8 +1679,14 @@ recv(TPid, Pid, TRef, Ref) -> %% send/2 -send(Pid, Pkt) -> - Pid ! {send, Pkt}. +send(Pid, Pkt) -> %% Strip potentially large message terms. + #diameter_packet{header = H, + bin = Bin, + transport_data = T} + = Pkt, + Pid ! {send, #diameter_packet{header = H, + bin = Bin, + transport_data = T}}. %% retransmit/4 -- cgit v1.2.3 From 1590920c910c030369fbf871b63f6836b988e90a Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 12 Feb 2015 11:41:59 +0100 Subject: Add service_opt() string_decode To control whether stringish Diameter types are decoded to string or left as binary. The motivation is the same as in the parent commit: to avoid large strings being copied when incoming Diameter messages are passed between processes; or *if* in the case of messages destined for handle_request and handle_answer callbacks, since these are decoded in the dedicated processes that the callbacks take place in. It would be possible to do something about other messages without requiring an option, but disabling the decode is the most effective. The value is a boolean(), true being the default for backwards compatibility. Setting false causes both diameter_caps records and decoded messages to contain binary() in relevant places that previously had string(): diameter_app(3) callbacks need to be prepared for the change. The Diameter types affected are OctetString and the derived types that can contain arbitrarily large values: OctetString, UTF8String, DiameterIdentity, DiameterURI, IPFilterRule, and QoSFilterRule. Time and Address are unaffected. The DiameterURI decode has been redone using re(3), which both simplifies and does away with a vulnerability resulting from the conversion of arbitrary strings to atom. The solution continues the use and abuse of the process dictionary for encode/decode purposes, last seen in commit 0f9cdba. --- lib/diameter/src/base/diameter_traffic.erl | 57 ++++++++++++++++++++++-------- 1 file changed, 42 insertions(+), 15 deletions(-) (limited to 'lib/diameter/src/base/diameter_traffic.erl') diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index 18c1965f77..a9dc46ea31 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -77,7 +77,8 @@ {peerT :: ets:tid(), service_name :: diameter:service_name(), apps :: [#diameter_app{}], - sequence :: diameter:sequence()}). + sequence :: diameter:sequence(), + codec :: list()}). %% Record stored in diameter_request for each outgoing request. -record(request, @@ -92,11 +93,16 @@ %% # make_recvdata/1 %% --------------------------------------------------------------------------- -make_recvdata([SvcName, PeerT, Apps, Mask | _]) -> +make_recvdata([SvcName, PeerT, Apps, {_,_} = Mask | _]) -> %% from old code + make_recvdata([SvcName, PeerT, Apps, [{sequence, Mask}]]); + +make_recvdata([SvcName, PeerT, Apps, SvcOpts | _]) -> + {_,_} = Mask = proplists:get_value(sequence, SvcOpts), #recvdata{service_name = SvcName, peerT = PeerT, apps = Apps, - sequence = Mask}. + sequence = Mask, + codec = [T || {K,_} = T <- SvcOpts, K == string_decode]}. %% --------------------------------------------------------------------------- %% peer_up/1 @@ -270,8 +276,11 @@ recv_request(TPid, #diameter_packet{header = #diameter_header{application_id = Id}} = Pkt, Dict0, - #recvdata{peerT = PeerT, apps = Apps} + #recvdata{peerT = PeerT, + apps = Apps, + codec = Opts} = RecvData) -> + diameter_codec:setopts(Opts), send_A(recv_R(diameter_service:find_incoming_app(PeerT, TPid, Id, Apps), TPid, Pkt, @@ -279,7 +288,13 @@ recv_request(TPid, RecvData), TPid, Dict0, - RecvData). + RecvData); + +recv_request(TPid, Pkt, Dict0, RecvData) -> %% from old code + recv_request(TPid, + Pkt, + Dict0, + #recvdata{} = erlang:append_element(RecvData, [])). %% recv_R/5 @@ -1225,10 +1240,9 @@ answer_rc(_, _, Sent) -> send_R(SvcName, AppOrAlias, Msg, Opts, Caller) -> case pick_peer(SvcName, AppOrAlias, Msg, Opts) of - {{_,_,_} = Transport, Mask} -> + {Transport, Mask, SvcOpts} -> + diameter_codec:setopts(SvcOpts), send_request(Transport, Mask, Msg, Opts, Caller, SvcName); - false -> - {error, no_connection}; {error, _} = No -> No end. @@ -1290,6 +1304,8 @@ send_request({TPid, Caps, App} SvcName, []). +%% send_R/7 + send_R({send, Msg}, Pkt, Transport, Opts, Caller, SvcName, Fs) -> send_R(make_request_packet(Msg, Pkt), Transport, @@ -1550,7 +1566,9 @@ a(Hdr, SvcName, discard) -> %% timer value is ignored. This means that an answer could be accepted %% from a peer after timeout in the case of failover. -retransmit({{_,_,App} = Transport, _Mask}, Req, Opts, SvcName, Timeout) -> +%% retransmit/5 + +retransmit({{_,_,App} = Transport, _, _}, Req, Opts, SvcName, Timeout) -> try retransmit(Transport, Req, SvcName, Timeout) of T -> recv_A(Timeout, SvcName, App, Opts, T) catch @@ -1571,17 +1589,26 @@ pick_peer(SvcName, pick_peer(SvcName, App, Msg, Opts#options{extra = []}); pick_peer(_, _, undefined, _) -> - false; + {error, no_connection}; pick_peer(SvcName, AppOrAlias, Msg, #options{filter = Filter, extra = Xtra}) -> - diameter_service:pick_peer(SvcName, - AppOrAlias, - {fun(D) -> get_destination(D, Msg) end, - Filter, - Xtra}). + pick(diameter_service:pick_peer(SvcName, + AppOrAlias, + {fun(D) -> get_destination(D, Msg) end, + Filter, + Xtra})). + +pick({{_,_,_} = Transport, Mask}) -> %% from old code; dialyzer complains + {Transport, Mask, []}; %% about this + +pick(false) -> + {error, no_connection}; + +pick(T) -> + T. %% handle_error/4 -- cgit v1.2.3 From b8a7df45c9e57a832f7db9b9b875b31d0ab7d29c Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Fri, 20 Mar 2015 02:18:06 +0100 Subject: Adapt to changed DiameterURI defaults in RFC 6733 Despite claims of full backwards compatibility, the text of RFC 6733 changes the interpretation of unspecified values in a DiameterURI. In particular, 3588 says that the default port and transport are 3868 and sctp respectively, while 6733 says it's either 3868/tcp (aaa) or 5658/tcp (aaas). The 3588 defaults were used regardless, but now use them only if the common dictionary is diameter_gen_base_rfc3588. The 6733 defaults are used otherwise. This kind of change in the standard can lead to interop problems, since a node has to know which RFC its peer is following to know that it will properly interpret missing URI components. Encode of a URI includes all components to avoid such confusion. That said, note that the defaults in the diameter_uri record have *not* been changed. This avoids breaking code that depends on them, but the risk is that such code sends inappropriate values. The record defaults may be changed in a future release, to force values to be explicitly specified. --- lib/diameter/src/base/diameter_traffic.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/diameter/src/base/diameter_traffic.erl') diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index c1c5a35531..784f9ca08f 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -280,7 +280,7 @@ recv_request(TPid, apps = Apps, codec = Opts} = RecvData) -> - diameter_codec:setopts(Opts), + diameter_codec:setopts([{common_dictionary, Dict0} | Opts]), send_A(recv_R(diameter_service:find_incoming_app(PeerT, TPid, Id, Apps), TPid, Pkt, -- cgit v1.2.3