diff --git a/apps/emqx/src/emqx_schema.erl b/apps/emqx/src/emqx_schema.erl index a673fa898..6f935f1e5 100644 --- a/apps/emqx/src/emqx_schema.erl +++ b/apps/emqx/src/emqx_schema.erl @@ -2608,7 +2608,7 @@ non_empty_string(_) -> {error, invalid_string}. servers_sc(Meta0, ParseOpts) -> %% if this filed has a default value %% then it is not NOT required - %% NOTE: maps:is_key is not the solution beause #{default => undefined} is legit + %% NOTE: maps:is_key is not the solution because #{default => undefined} is legit HasDefault = (maps:get(default, Meta0, undefined) =/= undefined), Required = maps:get(required, Meta0, not HasDefault), Meta = #{ @@ -2661,17 +2661,18 @@ normalize_host_port_str(Str) -> %% NOTE: Validator is called after converter. servers_validator(Opts, Required) -> fun(Str0) -> - Str = str(Str0), - case Str =:= "" orelse Str =:= "undefined" of - true when Required -> - %% it's a required field - %% but value is set to an empty string (from environment override) - %% or when the filed is not set in config file + case str(Str0) of + "" -> + %% Empty string is not allowed even if the field is not required + %% we should remove field from config if it's empty + throw("cannot_be_empty"); + "undefined" when Required -> + %% when the filed is not set in config file %% NOTE: assuming nobody is going to name their server "undefined" throw("cannot_be_empty"); - true -> + "undefined" -> ok; - _ -> + Str -> %% it's valid as long as it can be parsed _ = parse_servers(Str, Opts), ok @@ -2816,20 +2817,17 @@ is_port_number(Port) -> end. parse_port(Port) -> - try - P = list_to_integer(string:strip(Port)), - true = (P > 0), - true = (P =< 65535), - P - catch - _:_ -> - throw("bad_port_number") + case string:to_integer(string:strip(Port)) of + {P, ""} when P < 0 -> throw("port_number_must_be_positive"); + {P, ""} when P > 65535 -> throw("port_number_too_large"); + {P, ""} -> P; + _ -> throw("bad_port_number") end. quic_feature_toggle(Desc) -> sc( %% true, false are for user facing - %% 0, 1 are for internal represtation + %% 0, 1 are for internal representation typerefl:alias("boolean", typerefl:union([true, false, 0, 1])), #{ desc => Desc, diff --git a/apps/emqx/test/emqx_channel_SUITE.erl b/apps/emqx/test/emqx_channel_SUITE.erl index c3f27269b..6dd389350 100644 --- a/apps/emqx/test/emqx_channel_SUITE.erl +++ b/apps/emqx/test/emqx_channel_SUITE.erl @@ -1243,8 +1243,8 @@ session(InitFields) when is_map(InitFields) -> ), Session = emqx_session:init(Conf), maps:fold( - fun(Field, Value, Session) -> - emqx_session:set_field(Field, Value, Session) + fun(Field, Value, SessionAcc) -> + emqx_session:set_field(Field, Value, SessionAcc) end, Session, InitFields diff --git a/apps/emqx/test/emqx_schema_tests.erl b/apps/emqx/test/emqx_schema_tests.erl index e1ac1874f..a0d264662 100644 --- a/apps/emqx/test/emqx_schema_tests.erl +++ b/apps/emqx/test/emqx_schema_tests.erl @@ -455,10 +455,11 @@ servers_validator_test() -> NotRequired = emqx_schema:servers_validator(#{}, false), ?assertThrow("cannot_be_empty", Required("")), ?assertThrow("cannot_be_empty", Required(<<>>)), + ?assertThrow("cannot_be_empty", NotRequired("")), + ?assertThrow("cannot_be_empty", NotRequired(<<>>)), ?assertThrow("cannot_be_empty", Required(undefined)), - ?assertEqual(ok, NotRequired("")), - ?assertEqual(ok, NotRequired(<<>>)), ?assertEqual(ok, NotRequired(undefined)), + ?assertEqual(ok, NotRequired("undefined")), ok. converter_invalid_input_test() -> diff --git a/apps/emqx/test/emqx_session_SUITE.erl b/apps/emqx/test/emqx_session_SUITE.erl index 95d94707c..21d8f0a2a 100644 --- a/apps/emqx/test/emqx_session_SUITE.erl +++ b/apps/emqx/test/emqx_session_SUITE.erl @@ -471,8 +471,8 @@ session(InitFields) when is_map(InitFields) -> ), Session = emqx_session:init(Conf), maps:fold( - fun(Field, Value, Session) -> - emqx_session:set_field(Field, Value, Session) + fun(Field, Value, SessionAcc) -> + emqx_session:set_field(Field, Value, SessionAcc) end, Session, InitFields diff --git a/apps/emqx_statsd/test/emqx_statsd_SUITE.erl b/apps/emqx_statsd/test/emqx_statsd_SUITE.erl index a203ef7d5..bcc710050 100644 --- a/apps/emqx_statsd/test/emqx_statsd_SUITE.erl +++ b/apps/emqx_statsd/test/emqx_statsd_SUITE.erl @@ -33,6 +33,26 @@ "tags {\"t1\" = \"good\", test = 100}\n" "}\n" >>). +-define(BAD_CONF, << + "\n" + "statsd {\n" + "enable = true\n" + "flush_time_interval = 4s\n" + "sample_time_interval = 4s\n" + "server = \"\"\n" + "tags {\"t1\" = \"good\", test = 100}\n" + "}\n" +>>). + +-define(DEFAULT_CONF, << + "\n" + "statsd {\n" + "enable = true\n" + "flush_time_interval = 4s\n" + "sample_time_interval = 4s\n" + "tags {\"t1\" = \"good\", test = 100}\n" + "}\n" +>>). init_per_suite(Config) -> emqx_common_test_helpers:start_apps( @@ -55,6 +75,33 @@ set_special_configs(_) -> all() -> emqx_common_test_helpers:all(?MODULE). +t_server_validator(_) -> + Server0 = emqx_conf:get_raw([statsd, server]), + ?assertThrow( + #{ + kind := validation_error, + path := "statsd.server", + reason := "cannot_be_empty", + value := "" + }, + emqx_common_test_helpers:load_config(emqx_statsd_schema, ?BAD_CONF, #{ + raw_with_default => true + }) + ), + %% default + ok = emqx_common_test_helpers:load_config(emqx_statsd_schema, ?DEFAULT_CONF, #{ + raw_with_default => true + }), + undefined = emqx_conf:get_raw([statsd, server], undefined), + ?assertMatch("127.0.0.1:8125", emqx_conf:get([statsd, server])), + %% recover + ok = emqx_common_test_helpers:load_config(emqx_statsd_schema, ?BASE_CONF, #{ + raw_with_default => true + }), + Server2 = emqx_conf:get_raw([statsd, server]), + ?assertMatch(Server0, Server2), + ok. + t_statsd(_) -> {ok, Socket} = gen_udp:open(8126, [{active, true}]), receive @@ -137,7 +184,16 @@ t_config_update(_) -> ?assertNotEqual(OldPid, NewPid) after {ok, _} = emqx_statsd_config:update(OldRawConf) - end. + end, + %% bad server url + BadRawConf = OldRawConf#{<<"server">> := <<"">>}, + {error, #{ + kind := validation_error, + path := "statsd.server", + reason := "cannot_be_empty", + value := "" + }} = emqx_statsd_config:update(BadRawConf), + ok. request(Method) -> request(Method, []). diff --git a/changes/ce/fix-10119.en.md b/changes/ce/fix-10119.en.md new file mode 100644 index 000000000..c23a9dcdb --- /dev/null +++ b/changes/ce/fix-10119.en.md @@ -0,0 +1 @@ +Fix crash when `statsd.server` is set to an empty string. diff --git a/changes/ce/fix-10119.zh.md b/changes/ce/fix-10119.zh.md new file mode 100644 index 000000000..c77b99025 --- /dev/null +++ b/changes/ce/fix-10119.zh.md @@ -0,0 +1 @@ +修复 `statsd.server` 配置为空字符串时启动崩溃的问题。