Merge branch 'anders/diameter/M-bit/OTP-12947' into maint-17

* anders/diameter/M-bit/OTP-12947:
  Add service_opt() strict_mbit

7 files changed, 65 insertions, 8 deletions

diff --git a/lib/diameter/doc/src/diameter.xml b/lib/diameter/doc/src/diameter.xml
index 854bc5b432..b0cff32c9a 100644
--- a/lib/diameter/doc/src/diameter.xml
+++ b/lib/diameter/doc/src/diameter.xml
@@ -912,6 +912,49 @@ Options <c>monitor</c> and <c>link</c> are ignored.</p>
 Defaults to the empty list.</p>
 </item>
 
+<marker id="strict_mbit"/>
+<tag><c>{strict_mbit, boolean()}</c></tag>
+<item>
+<p>
+Whether or not to regard an AVP setting the M-bit as erroneous when
+the command grammar in question does not explicitly allow the AVP.
+If <c>true</c> then such AVPs are regarded as 5001 errors,
+DIAMETER_AVP_UNSUPPORTED.
+If <c>false</c> then the M-bit is ignored and policing
+it becomes the receiver's responsibility.</p>
+
+<p>
+Defaults to <c>true</c>.</p>
+
+<warning>
+<p>
+RFC 6733 is unclear about the semantics of the M-bit.
+One the one hand, the CCF specification in section 3.2 documents AVP
+in a command grammar as meaning <b>any</b> arbitrary AVP; on the
+other hand, 1.3.4 states that AVPs setting the M-bit cannot be added
+to an existing command: the modified command must instead be
+placed in a new Diameter application.</p>
+<p>
+The reason for the latter is presumably interoperability:
+allowing arbitrary AVPs setting the M-bit in a command makes its
+interpretation implementation-dependent, since there's no
+guarantee that all implementations will understand the same set of
+arbitrary AVPs in the context of a given command.
+However, interpreting <c>AVP</c> in a command grammar as <b>any</b>
+AVP, regardless of M-bit, renders 1.3.4 meaningless, since the receiver
+can simply ignore any AVP it thinks isn't relevant, regardless of the
+sender's intent.</p>
+<p>
+Beware of confusing mandatory in the sense of the M-bit with mandatory
+in the sense of the command grammar.
+The former is a semantic requirement: that the receiver understand the
+semantics of the AVP in the context in question.
+The latter is a syntactic requirement: whether or not the AVP must
+occur in the message in question.</p>
+</warning>
+
+</item>
+
 <marker id="string_decode"/>
 <tag><c>{string_decode, boolean()}</c></tag>
 <item>
diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl
index ac2126cdc5..39ccdc9873 100644
--- a/lib/diameter/include/diameter_gen.hrl
+++ b/lib/diameter/include/diameter_gen.hrl
@@ -30,7 +30,10 @@
 
 %% Key to a value in the process dictionary that determines whether or
 %% not an unrecognized AVP setting the M-bit should be regarded as an
-%% error or not. See is_strict/0.
+%% error or not. See is_strict/0. This is only used to relax M-bit
+%% interpretation inside Grouped AVPs not setting the M-bit. The
+%% service_opt() strict_mbit can be used to disable the check
+%% globally.
 -define(STRICT_KEY, strict).
 
 %% Key that says whether or not we should do a best-effort decode
@@ -447,7 +450,8 @@ relax(_, _) ->
     false.
 
 is_strict() ->
-    false /= getr(?STRICT_KEY).
+    diameter_codec:getopt(strict_mbit)
+        andalso false /= getr(?STRICT_KEY).
 
 %% relax/1
 %%
diff --git a/lib/diameter/src/base/diameter.erl b/lib/diameter/src/base/diameter.erl
index 010f977b97..9d71bcbbf8 100644
--- a/lib/diameter/src/base/diameter.erl
+++ b/lib/diameter/src/base/diameter.erl
@@ -311,6 +311,7 @@ call(SvcName, App, Message) ->
     | {sequence, sequence() | evaluable()}
     | {share_peers, remotes()}
     | {string_decode, boolean()}
+    | {strict_mbit, boolean()}
     | {incoming_maxlen, message_length()}
     | {use_shared_peers, remotes()}
     | {spawn_opt, list()}.
diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl
index f900bb0c5e..aab8b5887e 100644
--- a/lib/diameter/src/base/diameter_codec.erl
+++ b/lib/diameter/src/base/diameter_codec.erl
@@ -76,9 +76,10 @@ setopts(Opts)
   when is_list(Opts) ->
     lists:foreach(fun setopt/1, Opts).
 
-%% Decode stringish types to string()? The default true is for
-%% backwards compatibility.
-setopt({string_decode = K, false = B}) ->
+%% The default string_decode true is for backwards compatibility.
+setopt({K, false = B})
+  when K == string_decode;
+       K == strict_mbit ->
     setopt(K, B);
 
 %% Regard anything but the generated RFC 3588 dictionary as modern.
@@ -96,7 +97,8 @@ setopt(Key, Value) ->
 
 getopt(Key) ->
     case get({diameter, Key}) of
-        undefined when Key == string_decode ->
+        undefined when Key == string_decode;
+                       Key == strict_mbit ->
             true;
         undefined when Key == rfc ->
             6733;
diff --git a/lib/diameter/src/base/diameter_config.erl b/lib/diameter/src/base/diameter_config.erl
index 89b26707ad..242b6b4d08 100644
--- a/lib/diameter/src/base/diameter_config.erl
+++ b/lib/diameter/src/base/diameter_config.erl
@@ -646,6 +646,7 @@ make_config(SvcName, Opts) ->
                          {?NOMASK, sequence},
                          {nodes, restrict_connections},
                          {16#FFFFFF, incoming_maxlen},
+                         {true, strict_mbit},
                          {true, string_decode},
                          {[], spawn_opt}]),
 
@@ -684,12 +685,14 @@ opt(K, false = B)
        K == use_shared_peers;
        K == monitor;
        K == restrict_connections;
+       K == strict_mbit;
        K == string_decode ->
     B;
 
 opt(K, true = B)
   when K == share_peers;
        K == use_shared_peers;
+       K == strict_mbit;
        K == string_decode ->
     B;
 
diff --git a/lib/diameter/src/base/diameter_service.erl b/lib/diameter/src/base/diameter_service.erl
index a31cef2c8c..7d4a24f7a0 100644
--- a/lib/diameter/src/base/diameter_service.erl
+++ b/lib/diameter/src/base/diameter_service.erl
@@ -128,6 +128,7 @@
              | {share_peers, diameter:remotes()}       %% broadcast to
              | {use_shared_peers, diameter:remotes()}  %% use from
              | {restrict_connections, diameter:restriction()}
+             | {strict_mbit, boolean()}
              | {string_decode, boolean()}
              | {incoming_maxlen, diameter:message_length()}]}).
 %% shared_peers reflects the peers broadcast from remote nodes.
@@ -697,7 +698,8 @@ service_options(Opts) ->
                                                 ?RESTRICT)},
      {spawn_opt, proplists:get_value(spawn_opt, Opts, [])},
      {string_decode, proplists:get_value(string_decode, Opts, true)},
-     {incoming_maxlen, proplists:get_value(incoming_maxlen, Opts, 16#FFFFFF)}].
+     {incoming_maxlen, proplists:get_value(incoming_maxlen, Opts, 16#FFFFFF)},
+     {strict_mbit, proplists:get_value(strict_mbit, Opts, true)}].
 %% The order of options is significant since we match against the list.
 
 mref(false = No) ->
diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl
index 230a05fa11..6be2c35d48 100644
--- a/lib/diameter/src/base/diameter_traffic.erl
+++ b/lib/diameter/src/base/diameter_traffic.erl
@@ -79,6 +79,7 @@
          apps         :: [#diameter_app{}],
          sequence     :: diameter:sequence(),
          codec        :: [{string_decode, boolean()}
+                          | {strict_mbit, boolean()}
                           | {incoming_maxlen, diameter:message_length()}]}).
 %% Note that incoming_maxlen is currently handled in diameter_peer_fsm,
 %% so that any message exceeding the maximum is discarded. Retain the
@@ -108,7 +109,8 @@ make_recvdata([SvcName, PeerT, Apps, SvcOpts | _]) ->
               sequence = Mask,
               codec = [T || {K,_} = T <- SvcOpts,
                             lists:member(K, [string_decode,
-                                             incoming_maxlen])]}.
+                                             incoming_maxlen,
+                                             strict_mbit])]}.
 
 %% ---------------------------------------------------------------------------
 %% peer_up/1