| Age | Commit message (Collapse) | Author |
|---|
|
* anders/diameter/grouped_errors/OTP-12930:
Fix decode of Grouped AVPs containing errors
Simplify logic
Simplify logic
|
|
This has had a hugely negative impact on performance when decoding
messages containing many AVP: each decode of an AVP having variable
arity computed the length of the list of previously decoded AVPs when
checking that the allowed arity was not exceeded, even if the allowed
arity was infinite, making for O(n^2) cost. Here are some execution
times, for diameter_codec:decode/2 on a representative message with n
integer AVPs in the Common application (on the host at hand):
Before After
------- ---------
n = 1K 5 ms 2 ms
n = 10K 500 ms 25 ms
n = 100K 75 sec 225 ms
n = 1M 2.6 sec
Note the nearly linear increase following the change.
Remove the dire documentation warning for incoming_maxlen as a
consequence. It can still be useful to set, but not doing so won't have
the same consequences as previously.
|
|
Since the list can potentially be long.
|
|
The decode of an incoming Diameter message uses the record
representation to determine whether or not an AVP has been received with
the expected arity, the number of AVPs in each field following decode
being compared with the arity specified in the message grammar. The
problem with this is that decode failure isn't reflected in the record
representation, so that an AVP can be appended to the errors field of a
diameter_packet record despite an entry for the same AVP already
existing. This isn't a fault as much as a misleading error indication,
but now only append AVPs that aren't already represented.
|
|
RFC 6733 says this of Failed-AVP in 7.5:
In the case where the offending AVP is embedded within a Grouped AVP,
the Failed-AVP MAY contain the grouped AVP, which in turn contains
the single offending AVP. The same method MAY be employed if the
grouped AVP itself is embedded in yet another grouped AVP and so on.
In this case, the Failed-AVP MAY contain the grouped AVP hierarchy up
to the single offending AVP. This enables the recipient to detect
the location of the offending AVP when embedded in a group.
It says this of DIAMETER_INVALID_AVP_LENGTH in 7.1.5:
The request contained an AVP with an invalid length. A Diameter
message indicating this error MUST include the offending AVPs
within a Failed-AVP AVP. In cases where the erroneous AVP length
value exceeds the message length or is less than the minimum AVP
header length, it is sufficient to include the offending AVP
header and a zero filled payload of the minimum required length
for the payloads data type. If the AVP is a Grouped AVP, the
Grouped AVP header with an empty payload would be sufficient to
indicate the offending AVP. In the case where the offending AVP
header cannot be fully decoded when the AVP length is less than
the minimum AVP header length, it is sufficient to include an
offending AVP header that is formulated by padding the incomplete
AVP header with zero up to the minimum AVP header length.
The AVPs placed in the errors field of a diameter_packet record are
intended to be appropriate for inclusion in a Failed-AVP, but neither of
the above paragraphs has been followed in the Grouped case: the entire
faulty AVP (non-faulty components and all) has been included. This made
it impossible to identify the actual faulty AVP in all but simple case.
This commit adapts the decode to the RFC, and implements the suggested
single faulty AVP, nested in as many Grouped containers as required.
The best-effort decode of Failed-AVP in answer messages, initially
implemented in commit 0f9cdbaf, is also applied.
|
|
Testing is_failed() is unnecessary since put/2 a second time will
return a previously put 'true'.
|
|
Failed == undefined implies is_failed() == true. This was true even when
the code was written, in commit c2c00fdd.
|
|
In the case of a faulty AVP Length (pointing past the end of a message
or not spanning the header), an extra bit is prepended to data bytes in
diameter_avp:collect_avps/1 in order to force a 5014 decode error. The
bit is supposed to be removed as part of the decode in diameter_gen.hrl
but this didn't happen in case of an AVP that unknown to the dictionary
in question.
|
|
The function is intended to be traced on, to see abnormalities (mostly)
without producing excessive output. In the case of decode failure, the
error reason can be things like {badmatch, HugeBinary}.
Missed in commit 0058430.
|
|
The decode of a Grouped AVP ignored the case that extracting component
AVPs with diameter_codec:collect_avps/1 returned a tuple, in the case of
a truncated AVP header.
|
|
The AVPs of an incoming Diameter message diameter_codec:decode/2,3 are
decoded into a diameter_packet record in two ways: as a message-specific
record in the 'msg' field and as a deep list of diameter_avp records in
the 'avps' field. The record decode came first; the diameter_avp decode
came later to support the Diameter relay application, but can also be
convenient for non-relay applications. The diameter_avp representation
can be used with outgoing messages, but what exactly is supported for
isn't clearly documented.
In the diameter_avp list representation, it's AVPs of type Grouped that
lead to nesting: instead of a diameter_avp record, a Grouped AVP is
represented by a diameter_avp list whose head is the Grouped AVP itself,
and whose tail is the list of component AVPs.
The diameter_avp decode was broken in the case of decode errors: the
Grouped AVP was represented as a bare diameter_avp, and the component
records were lost. The decode now produces the intended list. Note that
component AVPs that could not be decoded will have 'undefined' in their
data field.
|
|
Decode can span multiple codec modules, so written entries cannot be
tagged on ?MODULE.
|
|
The bit is added in diameter_codec to induce a decode error in the case
of 5014 errors, but was not removed before returning the decoded result.
Code examining the binary data in a diameter_avp record would then see
the extra bit.
|
|
Commit c2c00fdd didn't get it quite right: it only decoded failed AVPs
in the common dictionary since it's this dictionary an answer-message is
decoded in. An extra dictionary isn't something that's easily passed
through the decode without rewriting dictionary compilation however, and
that's no small job, so continue with the use/abuse of the process
dictionary by storing the dictionary module for the decode to retrieve.
This is one step worse than previous uses since the dictionary is put in
one module (diameter_codec) and got in another (the dictionary module),
but it's the lesser of two evils.
|
|
Commit 066544fa had the unintended consequence of breaking the decode of
Failed-AVP in answer-message as defined in the RFC 3588, since the
grammar doesn't list Failed-AVP as an explicit component AVP, in
contrast to the RFC 6733 grammar, which does. Handle this case
explicitly, as an exception, just as with Failed-AVP as parent AVP.
|
|
* anders/diameter/hardening/OTP-11721:
Change answer_errors default from report to discard
|
|
In the same vein as commit 00584303, to avoid logging traffic-related
happenings.
Not that the value in diameter.hrl is just documentation: the value is
set explicitly when diameter:start_service/2 creates diameter_app
records.
|
|
Commit 4ce2d3a6 (diameter-1.4.2, OTP-11007) disabled the decode of
values in Failed-AVP components since any error caused the decode of
Failed-AVP itself to fail. This is less than useful since (1) we should
be able to decode it given that we've sent it (modulo mangling on the
way to the peer and back), and (2) it's not unheard of to examine
Failed-AVP to see what the peer objected to.
This commits adds a best-effort decode: decode if possible, otherwise
not, using the same abuse of the process dictionary as commit bbdb027c.
|
|
Commit 4ce2d3a6 added the insertion of a single bit into binary AVP data
to induce an encode error in the case of a header length that pointed
past the available bytes: a 5014 = DIAMETER_INVALID_AVP_LENGTH error.
Commit 838856b fixed this for stringish Diameter types, but both commits
neglected the case in which the offending AVP isn't known to the
dictionary in question. Unless the AVP was regarded as erroneous for
other reasons (eg. an M-bit resulting in 5001) it would be happily be
packed into an 'AVP' field. If it was regarded as an error, the record
could be passed back to diameter_codec:pack_avp/1, and if the record
contained header data then there was no clause to deal with the
unpleasantry.
Deal with it by having the dictionary module strip the extra bit and
flag the AVP as 5014, and by having diameter_codec handle any extra bit
coming from an dictionary compiled against an old diameter_gen. An old
dictionary won't detect 5014 however, so dictionaries should be
recompiled.
Change most of the guards in diameter_codec from is_bitstring/1 to
is_binary/1. What's being passed to the decode functions are binaries
received other the network. The only case in which a non-binary
bitstring is when we've placed an extra bit there ourselves. (Modulo
someone doing something they shouldn't.)
|
|
The former were a little over-enthusiastic and could cause a node to be
logged to death if a peer Diameter node was sufficiently ill-willed.
The function calls are to diameter_lib:log/4, the arguments of which
identify the happening in question, and which does nothing but provide a
function to trace on. Many existing log calls have been shrunk.
The only remaining traffic-related report (hopefully) is that resulting
from {answer_errors, report} config, and this has been slimmed.
|
|
* anders/diameter/17.0_release/OTP-11605:
Fix diameter.hrl comment typos
|
|
A Diameter Header Command Code is 24 bits, not 8, and an Application-ID
is 32 bits, not 24.
Thanks to Austin Aigbe for pointing it out.
|
|
RFC 6733 says this, in 4.4:
Receivers of a Grouped AVP that does not have the 'M' (mandatory) bit
set and one or more of the encapsulated AVPs within the group has the
'M' (mandatory) bit set MAY simply be ignored if the Grouped AVP itself
is unrecognized. The rule applies even if the encapsulated AVP with its
'M' (mandatory) bit set is further encapsulated within other sub-groups,
i.e., other Grouped AVPs embedded within the Grouped AVP.
The first sentence is mangled but take it to mean this:
An unrecognized AVP of type Grouped that does not set the 'M' bit MAY
be ignored even if one of its encapsulated AVPs sets the 'M' bit.
This is a bit of a non-statement since if the AVP is unrecognized then
its type is unknown. We therefore don't know that its data bytes contain
encapsulated AVPs, so can't but ignore any of those that set the M-bit.
Doing anything else when the type *is* known would be inconsistent.
OTP-11087 (commit 066544fa) caused the M-bit on any unrecognized AVP to
be regarded as an error, unrecognized being taken to mean "not
explicitly defined as a member of its container". (That is, an AVP that
can't be packed into a dedicated record field, which is slightly
stronger than "not defined".) This fixed the original intention for
top-level AVPs but broke the required leniency for Grouped AVPs whose
type is known. This commit restores the leniency.
Note that dictionary files need to be recompiled for the commit to have
effect.
Thanks to Rory McKeown for reporting the problem.
|
|
An AVP setting the M-bit was not regarded as erroneous if it was defined
in the dictionary in question and its container (message or Grouped AVP)
had an 'AVP' field. It's now regarded as a 5001 error (AVP_UNSUPPORTED),
as in the case that the AVP is not defined.
|
|
The previous commit ensures that only one will be reported in an answer
message when diameter itself sets Result-Code/Failed-AVP.
The order of errors in #diameter_packet.errors is that in which they're
detected, not the reverse as previously.
|
|
|
|
Invalid lengths come in two flavours: ones that correctly point at the
end of an AVP's payload but don't agree with its type, and ones that
point elsewhere. The former are relatively harmless but the latter leave
no way to recover AVP boundaries, which typically results in failure to
decode subsequent AVP's in the message in question.
In the case that AVP Length points past the end of the message, diameter
incorrectly regarded the error as 5009, INVALID_AVP_BITS: not right
since the error has nothing to do with AVP Flags. Ditto if the length
was less than 8, a minimal header length. Only in the remaining case was
the detected error 5014, INVALID_AVP_LENGTH. However, in this case it
slavishly followed RFC 3588 in suggesting the undecodable AVP as
Failed-AVP, thereby passing the woeful payload back to the sender to
have equal difficulty decoding. Now follow RFC 6733 and suggest an AVP
with a zero-filled payload.
|
|
RFC 3588 allowed only 3xxx result codes in an answer-message (that is,
an answer that sets the E-bit) while RFC 6733 also allows 5xxx result
codes. Setting request_errors = answer tells diameter to answer 5xxx
errors itself. Returning {answer_message, integer()} from a
handle_request callback allows both 3xxx and 5xxx result codes to be
set. {protocol_error, integer()} is retained for 3xxx result codes.
|
|
Configuring the value 'callback' all errors detected in incoming
requests to result in a handle_request callback. The default value
'answer_3xxx' is the previous behaviour in which diameter answers
protocol errors without a callback.
|
|
|
|
Instead, use whatever dictionary a transport has configured as
supporting application id 0. This is to support the updated RFC 6733
dictionaries (which bring with them updated records) and also to be able
to transparently support any changed semantics (eg. 5xxx in
answer-message).
|
|
|
|
To make for easier adding of future options. The record is only passed
into transport modules so the only compatibility issue is with these.
(No issue for diameter_{tcp,sctp} and unlikely but theoretically
possible for any other implementations, which probably don't exist at
this point.)
|
|
The module was originally just intended as a minimal callback
implementation that could be used as a template. Being able to order
just a subset of callbacks (with reasonable defaults) makes for
simpler code in many cases however so ready support for this can be
useful.
|
|
Should have been included in 5af64c7d57d83ce35bfd7b15ac3ce6ec7459fd73.
|
|
The message being encoded and dictionary module are included
by diameter_codec.erl so diameter_gen.hrl doing so was overkill.
|
|
|
|
The application provides an implementation of the Diameter protocol
as defined in RFC 3588.
|
