From b4ecd2efd32df9d14d3314b705cd1114de7141ae Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Wed, 7 Dec 2011 11:50:22 +0100 Subject: Fix semantic checks on AVP qualifiers Didn't quite interpret '*' as RFC 3588 dictates. In particular, the interpretation depends on what's being qualified, a required, optional or fixed AVP. --- lib/diameter/src/compiler/diameter_codegen.erl | 10 +- lib/diameter/src/compiler/diameter_dict_util.erl | 83 +++++++++--- lib/diameter/test/diameter_compiler_SUITE.erl | 156 +++++++++++++++++++++-- 3 files changed, 211 insertions(+), 38 deletions(-) diff --git a/lib/diameter/src/compiler/diameter_codegen.erl b/lib/diameter/src/compiler/diameter_codegen.erl index 2c011376ad..489f521f70 100644 --- a/lib/diameter/src/compiler/diameter_codegen.erl +++ b/lib/diameter/src/compiler/diameter_codegen.erl @@ -835,15 +835,15 @@ avp_info(Entry) -> %% {Name, Arity} [A] -> {A, {0,1}}; {Q,T} -> {A,_} = avp_info(T), - {A, arity(Q)} + {A, arity(T,Q)} end. %% Normalize arity to 1 or {N,X} where N is an integer. A record field %% for an AVP is list-valued iff the normalized arity is not 1. -arity('*' = Inf) -> {0, Inf}; -arity({'*', N}) -> {0, N}; -arity({1,1}) -> 1; -arity(T) -> T. +arity({{_}}, '*' = Inf) -> {0, Inf}; +arity([_], '*' = Inf) -> {0, Inf}; +arity({_}, '*' = Inf) -> {1, Inf}; +arity(_, {_,_} = Q) -> Q. prefix(Spec) -> case orddict:find(prefix, Spec) of diff --git a/lib/diameter/src/compiler/diameter_dict_util.erl b/lib/diameter/src/compiler/diameter_dict_util.erl index 2207925e49..e4cd29ab7f 100644 --- a/lib/diameter/src/compiler/diameter_dict_util.erl +++ b/lib/diameter/src/compiler/diameter_dict_util.erl @@ -174,7 +174,13 @@ fmt(message_application_id_mismatch) -> fmt(invalid_avp_order) -> "AVP reference ~c~s~c at line ~p breaks fixed/required/optional order"; -fmt(invalid_qualifier) -> +fmt(required_avp_has_zero_max_arity) -> + "Required AVP has maximum arity 0 at line ~p"; +fmt(required_avp_has_zero_min_arity) -> + "Required AVP has minimum arity 0 at line ~p"; +fmt(optional_avp_has_nonzero_min_arity) -> + "Optional AVP has non-zero minimum arity at line ~p"; +fmt(qualifier_has_min_greater_than_max) -> "Qualifier ~p*~p at line ~p has Min > Max"; fmt(avp_already_referenced) -> "AVP ~s at line ~p already referenced at line ~p"; @@ -300,7 +306,7 @@ body(avp_types, {Name, Code, Type, Flags}) -> body(messages, {"answer-message", _, _, [], Avps}) -> [?NL, ?NL, ?INDENT, "answer-message ::= < Diameter Header: code, ERR [PXY] >", - f_avps(Avps)]; + f_avps(Avps)]; body(messages, {Name, Code, Flags, ApplId, Avps}) -> [?NL, ?NL, ?INDENT, word(Name), " ::= ", header(Code, Flags, ApplId), f_avps(Avps)]; @@ -326,7 +332,8 @@ f_avps(L) -> [[?NL, ?INDENT, ?INDENT, f_avp(A)] || A <- L]. f_avp({Q, A}) -> - f_avp(f_qual(Q), f_delim(A)); + [D | _] = Avp = f_delim(A), + f_avp(f_qual(D, Q), Avp); f_avp(A) -> f_avp("", f_delim(A)). @@ -337,17 +344,19 @@ f_delim({A}) -> f_delim([A]) -> [$[, word(A), $]]. -f_avp(Q, Avp) -> +f_avp(Q, [L, Avp, R]) -> Len = length(lists:flatten([Q])), - [io_lib:format("~*s", [-1*max(Len+1, 6) , Q]), Avp]. + [io_lib:format("~*s", [-1*max(Len+1, 6) , Q]), L, " ", Avp, " ", R]. -f_qual('*') -> +f_qual(${, '*') -> + "1*"; %% Equivalent to "*" but the more common/obvious rendition +f_qual(_, '*') -> "*"; -f_qual({'*', N}) -> +f_qual(_, {'*', N}) -> [$*, ?I(N)]; -f_qual({N, '*'}) -> +f_qual(_, {N, '*'}) -> [?I(N), $*]; -f_qual({M,N}) -> +f_qual(_, {M,N}) -> [?I(M), $*, ?I(N)]. section(Key) -> @@ -500,7 +509,17 @@ make_body(Avps) -> avp([false, D, Avp]) -> avp(D, Avp); avp([Q, D, Avp]) -> - {qual(Q), avp(D, Avp)}. + case {qual(D, Q), avp(D, Avp)} of + {{0,1}, A} when D == $[ -> + A; + {{1,1}, A} -> + A; + T -> + T + end. +%% Could just store the qualifier as a pair in all cases but the more +%% compact form is easier to parse visually so live with a bit of +%% mapping. Ditto the use of '*'. avp(D, {'AVP', _}) -> delim(D, "AVP"); @@ -514,14 +533,40 @@ delim(${, N) -> delim($[, N) -> [N]. -qual({true, {_,_,N}}) -> - {'*', N}; -qual({{_,_,N}, true}) -> +%% There's a difference between max = 0 and not specifying an AVP: +%% reception of an AVP with max = 0 will always be an error, otherwise +%% it depends on the existence of 'AVP' and the M flag. + +qual(${, {{_,L,0}, _}) -> + ?RETURN(required_avp_has_zero_min_arity, [L]); +qual(${, {_, {_,L,0}}) -> + ?RETURN(required_avp_has_zero_max_arity, [L]); + +qual($[, {{_,L,N}, _}) + when 0 < N -> + ?RETURN(optional_avp_has_nonzero_min_arity, [L]); + +qual(_, {{_,L,Min}, {_,_,Max}}) + when Min > Max -> + ?RETURN(qualifier_has_min_greater_than_max, [Min, Max, L]); + +qual(_, true) -> + '*'; + +qual(${, {true, {_,_,N}}) -> + {1, N}; +qual(_, {true, {_,_,N}}) -> + {0, N}; + +qual(D, {{_,_,N}, true}) + when D == ${, N == 1; + D /= ${, N == 0 -> + '*'; +qual(_, {{_,_,N}, true}) -> {N, '*'}; -qual({{_,_,N},{_,_,M}}) -> - {N, M}; -qual(true) -> - '*'. + +qual(_, {{_,_,Min}, {_,_,Max}}) -> + {Min, Max}. %% Optional reports when running verbosely. report(What, [F | A]) @@ -851,10 +896,6 @@ xa(Ds, [[Qual, D, {'AVP', Line}] | Avps], Dict, Key, Name) -> xa([], [[_Qual, D, {_, Line, Name}] | _], _, _, _) -> ?RETURN(invalid_avp_order, [D, Name, close(D), Line]); -xa([D|_], [[{{_, Line, Min}, {_, _, Max}}, D, _] | _], _, _, _) - when Min > Max -> - ?RETURN(invalid_qualifier, [Min, Max, Line]); - xa([D|_] = Ds, [[Qual, D, {_, Line, AvpName}] | Avps], Dict, Key, Name) -> xa(Ds, Avps, diff --git a/lib/diameter/test/diameter_compiler_SUITE.erl b/lib/diameter/test/diameter_compiler_SUITE.erl index cc4b1ddac5..8f563ff2ab 100644 --- a/lib/diameter/test/diameter_compiler_SUITE.erl +++ b/lib/diameter/test/diameter_compiler_SUITE.erl @@ -22,7 +22,6 @@ %% -module(diameter_compiler_SUITE). --compile({no_auto_import, [error/2]}). -export([suite/0, all/0, @@ -30,7 +29,7 @@ end_per_suite/1]). %% testcases --export([format/1, +-export([format/1, format/2, replace/1, replace/2]). -export([dict/0]). %% fake dictionary module @@ -46,7 +45,10 @@ %% returned in the first element of the error tuple returned by %% diameter_dict_util:parse/2. -define(REPLACE, - [{scan, + [{ok, + "", + ""}, + {scan, "@id 0", "@id \\&"}, {scan, @@ -154,9 +156,126 @@ {invalid_avp_order, "CEA ::=", "{Result-Code} &"}, - {invalid_qualifier, - "CEA ::=.*", - "& 3*2"}, + {ok, + "{ Product-Name", + "* &"}, + {required_avp_has_zero_max_arity, + "{ Product-Name", + "*0 &"}, + {required_avp_has_zero_min_arity, + "{ Product-Name", + "0* &"}, + {required_avp_has_zero_min_arity, + "{ Product-Name", + "0*0 &"}, + {ok, + "{ Product-Name", + "*1 &"}, + {ok, + "{ Product-Name", + "1* &"}, + {ok, + "{ Product-Name", + "1*1 &"}, + {ok, + "{ Product-Name", + "2* &"}, + {ok, + "{ Product-Name", + "*2 &"}, + {ok, + "{ Product-Name", + "2*2 &"}, + {ok, + "{ Product-Name", + "2*3 &"}, + {qualifier_has_min_greater_than_max, + "{ Product-Name", + "3*2 &"}, + {ok, + "\\[ Origin-State-Id", + "* &"}, + {ok, + "\\[ Origin-State-Id", + "0* &"}, + {ok, + "\\[ Origin-State-Id", + "*0 &"}, + {ok, + "\\[ Origin-State-Id", + "0*0 &"}, + {ok, + "\\[ Origin-State-Id", + "0*1 &"}, + {ok, + "\\[ Origin-State-Id", + "0*2 &"}, + {ok, + "\\[ Origin-State-Id", + "*1 &"}, + {optional_avp_has_nonzero_min_arity, + "\\[ Origin-State-Id", + "1* &"}, + {optional_avp_has_nonzero_min_arity, + "\\[ Origin-State-Id", + "1*1 &"}, + {ok, + "\\[ Origin-State-Id", + "*2 &"}, + {optional_avp_has_nonzero_min_arity, + "\\[ Origin-State-Id", + "2* &"}, + {optional_avp_has_nonzero_min_arity, + "\\[ Origin-State-Id", + "2*2 &"}, + {optional_avp_has_nonzero_min_arity, + "\\[ Origin-State-Id", + "2*3 &"}, + {optional_avp_has_nonzero_min_arity, + "\\[ Origin-State-Id", + "3*2 &"}, + {ok, + "^ *< Session-Id", + "* &"}, + {ok, + "^ *< Session-Id", + "*0 &"}, + {ok, + "^ *< Session-Id", + "0* &"}, + {ok, + "^ *< Session-Id", + "0*0 &"}, + {ok, + "^ *< Session-Id", + "0*1 &"}, + {ok, + "^ *< Session-Id", + "0*2 &"}, + {ok, + "^ *< Session-Id", + "*1 &"}, + {ok, + "^ *< Session-Id", + "1* &"}, + {ok, + "^ *< Session-Id", + "1*1 &"}, + {ok, + "^ *< Session-Id", + "*2 &"}, + {ok, + "^ *< Session-Id", + "2* &"}, + {ok, + "^ *< Session-Id", + "2*2 &"}, + {ok, + "^ *< Session-Id", + "2*3 &"}, + {qualifier_has_min_greater_than_max, + "^ *< Session-Id", + "3*2 &"}, {avp_already_referenced, "CER ::=.*", "& {Origin-Host}"}, @@ -232,7 +351,13 @@ end_per_suite(_Config) -> %% Ensure that parse o format is the identity map. format(Config) -> Bin = proplists:get_value(base, Config), - {ok, Dict} = diameter_dict_util:parse(Bin, []), + [] = ?util:run([{?MODULE, [format, M, Bin]} + || E <- ?REPLACE, + {ok, M} <- [norm(E)]]). + +format(Mods, Bin) -> + B = modify(Bin, Mods), + {ok, Dict} = diameter_dict_util:parse(B, []), {ok, D} = diameter_dict_util:parse(diameter_dict_util:format(Dict), []), {Dict, Dict} = {Dict, D}. @@ -240,13 +365,12 @@ format(Config) -> replace(Config) -> Bin = proplists:get_value(base, Config), - [] = ?util:run([{?MODULE, [replace, E, Bin]} || E <- ?REPLACE]). - -replace({E, RE, Repl}, Bin) -> - replace({E, [{RE, Repl}]}, Bin); + [] = ?util:run([{?MODULE, [replace, N, Bin]} + || E <- ?REPLACE, + N <- [norm(E)]]). replace({E, Mods}, Bin) -> - B = iolist_to_binary(lists:foldl(fun re/2, Bin, Mods)), + B = modify(Bin, Mods), case {E, diameter_dict_util:parse(B, [{include, here()}]), Mods} of {ok, {ok, Dict}, _} -> Dict; @@ -262,6 +386,14 @@ re({RE, Repl}, Bin) -> %% =========================================================================== +modify(Bin, Mods) -> + lists:foldl(fun re/2, Bin, Mods). + +norm({E, RE, Repl}) -> + {E, [{RE, Repl}]}; +norm({_,_} = T) -> + T. + nochar(Char, Str, Err) -> Err == parse orelse not lists:member(Char, Str) orelse Str. -- cgit v1.2.3