From e8cead48b211972b116cb4f3a5311d1c006719de Mon Sep 17 00:00:00 2001
From: Zhongwen Deng <zhongwencool@gmail.com>
Date: Thu, 28 Apr 2022 14:26:25 +0800
Subject: [PATCH] fix: bad plugin file crash

---
 .../src/emqx_mgmt_api_plugins.erl             | 37 ++++++++++++-------
 apps/emqx_plugins/src/emqx_plugins.erl        | 14 ++++---
 2 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/apps/emqx_management/src/emqx_mgmt_api_plugins.erl b/apps/emqx_management/src/emqx_mgmt_api_plugins.erl
index ac904f479..b36c1aae4 100644
--- a/apps/emqx_management/src/emqx_mgmt_api_plugins.erl
+++ b/apps/emqx_management/src/emqx_mgmt_api_plugins.erl
@@ -100,7 +100,9 @@ schema("/plugins/install") ->
             },
             responses => #{
                 200 => <<"OK">>,
-                400 => emqx_dashboard_swagger:error_codes(['UNEXPECTED_ERROR', 'ALREADY_INSTALLED'])
+                400 => emqx_dashboard_swagger:error_codes(
+                    ['UNEXPECTED_ERROR', 'ALREADY_INSTALLED', 'BAD_PLUGIN_INFO']
+                )
             }
         }
     };
@@ -319,20 +321,27 @@ upload_install(post, #{body := #{<<"plugin">> := Plugin}}) when is_map(Plugin) -
     %% emqx_plugins_monitor should copy plugins from other core node when boot-up.
     case emqx_plugins:describe(string:trim(FileName, trailing, ".tar.gz")) of
         {error, #{error := "bad_info_file", return := {enoent, _}}} ->
-            {AppName, _Vsn} = emqx_plugins:parse_name_vsn(FileName),
-            AppDir = filename:join(emqx_plugins:install_dir(), AppName),
-            case filelib:wildcard(AppDir ++ "*.tar.gz") of
-                [] ->
-                    do_install_package(FileName, Bin);
-                OtherVsn ->
+            case emqx_plugins:parse_name_vsn(FileName) of
+                {ok, AppName, _Vsn} ->
+                    AppDir = filename:join(emqx_plugins:install_dir(), AppName),
+                    case filelib:wildcard(AppDir ++ "*.tar.gz") of
+                        [] ->
+                            do_install_package(FileName, Bin);
+                        OtherVsn ->
+                            {400, #{
+                                code => 'ALREADY_INSTALLED',
+                                message => iolist_to_binary(
+                                    io_lib:format(
+                                        "~p already installed",
+                                        [OtherVsn]
+                                    )
+                                )
+                            }}
+                    end;
+                {error, Reason} ->
                     {400, #{
-                        code => 'ALREADY_INSTALLED',
-                        message => iolist_to_binary(
-                            io_lib:format(
-                                "~p already installed",
-                                [OtherVsn]
-                            )
-                        )
+                        code => 'BAD_PLUGIN_INFO',
+                        message => iolist_to_binary([Reason, ":", FileName])
                     }}
             end;
         {ok, _} ->
diff --git a/apps/emqx_plugins/src/emqx_plugins.erl b/apps/emqx_plugins/src/emqx_plugins.erl
index 0376eaf27..fec50f5f6 100644
--- a/apps/emqx_plugins/src/emqx_plugins.erl
+++ b/apps/emqx_plugins/src/emqx_plugins.erl
@@ -386,7 +386,7 @@ plugins_readme(_NameVsn, _Options, Info) ->
     Info.
 
 plugin_status(NameVsn, Info) ->
-    {AppName, _AppVsn} = parse_name_vsn(NameVsn),
+    {ok, AppName, _AppVsn} = parse_name_vsn(NameVsn),
     RunningSt =
         case application:get_key(AppName, vsn) of
             {ok, _} ->
@@ -437,7 +437,7 @@ check_plugin(
                 %% assert
                 [_ | _] = Apps,
                 %% validate if the list is all <app>-<vsn> strings
-                lists:foreach(fun parse_name_vsn/1, Apps)
+                lists:foreach(fun(App) -> {ok, _, _} = parse_name_vsn(App) end, Apps)
             catch
                 _:_ ->
                     throw(#{
@@ -471,7 +471,7 @@ load_code_start_apps(RelNameVsn, #{<<"rel_apps">> := Apps}) ->
     AppNames =
         lists:map(
             fun(AppNameVsn) ->
-                {AppName, AppVsn} = parse_name_vsn(AppNameVsn),
+                {ok, AppName, AppVsn} = parse_name_vsn(AppNameVsn),
                 EbinDir = filename:join([LibDir, AppNameVsn, "ebin"]),
                 ok = load_plugin_app(AppName, AppVsn, EbinDir, RunningApps),
                 AppName
@@ -559,7 +559,7 @@ ensure_apps_stopped(#{<<"rel_apps">> := Apps}) ->
     AppsToStop =
         lists:map(
             fun(NameVsn) ->
-                {AppName, _AppVsn} = parse_name_vsn(NameVsn),
+                {ok, AppName, _AppVsn} = parse_name_vsn(NameVsn),
                 AppName
             end,
             Apps
@@ -686,8 +686,10 @@ for_plugin(#{name_vsn := NameVsn, enable := false}, _Fun) ->
 parse_name_vsn(NameVsn) when is_binary(NameVsn) ->
     parse_name_vsn(binary_to_list(NameVsn));
 parse_name_vsn(NameVsn) when is_list(NameVsn) ->
-    {AppName, [$- | Vsn]} = lists:splitwith(fun(X) -> X =/= $- end, NameVsn),
-    {list_to_atom(AppName), Vsn}.
+    case lists:splitwith(fun(X) -> X =/= $- end, NameVsn) of
+        {AppName, [$- | Vsn]} -> {ok, list_to_atom(AppName), Vsn};
+        _ -> {error, "bad_name_vsn"}
+    end.
 
 pkg_file(NameVsn) ->
     filename:join([install_dir(), bin([NameVsn, ".tar.gz"])]).