From 5be4d97c42d5de33257b8a0c26ceef0b53a7433b Mon Sep 17 00:00:00 2001 From: Zhongwen Deng Date: Mon, 13 Mar 2023 12:08:19 +0800 Subject: [PATCH 1/4] fix: don't allow empty string in server_port schema --- apps/emqx/src/emqx_schema.erl | 34 ++++++------ apps/emqx_statsd/test/emqx_statsd_SUITE.erl | 58 ++++++++++++++++++++- 2 files changed, 73 insertions(+), 19 deletions(-) diff --git a/apps/emqx/src/emqx_schema.erl b/apps/emqx/src/emqx_schema.erl index a673fa898..914ad40a1 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 + %% 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_too_small"); + {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_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, []). From ee2847dcd916c56135bf5c1c996a6f77102d402f Mon Sep 17 00:00:00 2001 From: Zhongwen Deng Date: Mon, 13 Mar 2023 14:33:51 +0800 Subject: [PATCH 2/4] test: make schema test happy --- apps/emqx/src/emqx_schema.erl | 2 +- apps/emqx/test/emqx_channel_SUITE.erl | 4 ++-- apps/emqx/test/emqx_schema_tests.erl | 5 +++-- apps/emqx/test/emqx_session_SUITE.erl | 4 ++-- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/apps/emqx/src/emqx_schema.erl b/apps/emqx/src/emqx_schema.erl index 914ad40a1..1894d7fb5 100644 --- a/apps/emqx/src/emqx_schema.erl +++ b/apps/emqx/src/emqx_schema.erl @@ -2664,7 +2664,7 @@ servers_validator(Opts, Required) -> case str(Str0) of "" -> %% Empty string is not allowed even if the field is not required - %% remove field from config if it's empty + %% 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 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 From cec399c60252f771e130521a621689318d3daf72 Mon Sep 17 00:00:00 2001 From: Zhongwen Deng Date: Mon, 13 Mar 2023 14:44:49 +0800 Subject: [PATCH 3/4] chore: add changelog for statsd.server --- changes/ce/fix-10119.en.md | 1 + changes/ce/fix-10119.zh.md | 1 + 2 files changed, 2 insertions(+) create mode 100644 changes/ce/fix-10119.en.md create mode 100644 changes/ce/fix-10119.zh.md diff --git a/changes/ce/fix-10119.en.md b/changes/ce/fix-10119.en.md new file mode 100644 index 000000000..c88363869 --- /dev/null +++ b/changes/ce/fix-10119.en.md @@ -0,0 +1 @@ +Fix crash when `statsd.server` is empty diff --git a/changes/ce/fix-10119.zh.md b/changes/ce/fix-10119.zh.md new file mode 100644 index 000000000..31b233c04 --- /dev/null +++ b/changes/ce/fix-10119.zh.md @@ -0,0 +1 @@ +修复 `statsd.server` 为空时启动崩溃的问题 From 80205d970496f262bf977783d42c5646ca602fbf Mon Sep 17 00:00:00 2001 From: Zhongwen Deng Date: Tue, 14 Mar 2023 10:54:05 +0800 Subject: [PATCH 4/4] chore: apply code review --- apps/emqx/src/emqx_schema.erl | 2 +- changes/ce/fix-10119.en.md | 2 +- changes/ce/fix-10119.zh.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/emqx/src/emqx_schema.erl b/apps/emqx/src/emqx_schema.erl index 1894d7fb5..6f935f1e5 100644 --- a/apps/emqx/src/emqx_schema.erl +++ b/apps/emqx/src/emqx_schema.erl @@ -2818,7 +2818,7 @@ is_port_number(Port) -> parse_port(Port) -> case string:to_integer(string:strip(Port)) of - {P, ""} when P < 0 -> throw("port_number_too_small"); + {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") diff --git a/changes/ce/fix-10119.en.md b/changes/ce/fix-10119.en.md index c88363869..c23a9dcdb 100644 --- a/changes/ce/fix-10119.en.md +++ b/changes/ce/fix-10119.en.md @@ -1 +1 @@ -Fix crash when `statsd.server` is empty +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 index 31b233c04..c77b99025 100644 --- a/changes/ce/fix-10119.zh.md +++ b/changes/ce/fix-10119.zh.md @@ -1 +1 @@ -修复 `statsd.server` 为空时启动崩溃的问题 +修复 `statsd.server` 配置为空字符串时启动崩溃的问题。