[Tarantool-patches] [PATCH] lua: fix panic in case when log.cfg.log incorrecly specified

[Tarantool-patches] [PATCH] lua: fix panic in case when log.cfg.log incorrecly specified

[olegrok]

From: Oleg Babin <babinoleg@mail.ru>

This patch makes log.cfg{log = ...} behaviour the same as in
box.cfg{log = ...} and fixes panic if "log" is incorrectly
specified. For such purpose we export "say_parse_logger_type"
function and use for logger type validation and logger type
parsing.

Closes #5130
---
Issue: https://github.com/tarantool/tarantool/issues/5130 
Branch: https://github.com/tarantool/tarantool/tree/olegrok/5130-panic-on-invalid-log

 src/exports.h                                 |  1 +
 src/lua/log.lua                               | 28 ++++++++++++++++---
 .../gh-5130-panic-on-invalid-log.test.lua     | 23 +++++++++++++++
 3 files changed, 48 insertions(+), 4 deletions(-)
 create mode 100755 test/app-tap/gh-5130-panic-on-invalid-log.test.lua

diff --git a/src/exports.h b/src/exports.h
index 7f0601f4f..6d8303180 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -434,6 +434,7 @@ EXPORT(_say)
 EXPORT(say_logger_init)
 EXPORT(say_logger_initialized)
 EXPORT(say_logrotate)
+EXPORT(say_parse_logger_type)
 EXPORT(say_set_log_format)
 EXPORT(say_set_log_level)
 EXPORT(SHA1internal)
diff --git a/src/lua/log.lua b/src/lua/log.lua
index cfd83f592..62ea61f2d 100644
--- a/src/lua/log.lua
+++ b/src/lua/log.lua
@@ -54,6 +54,9 @@ ffi.cdef[[
     pid_t log_pid;
     extern int log_level;
     extern int log_format;
+
+    int
+    say_parse_logger_type(const char **str, enum say_logger_type *type);
 ]]
 
 local S_WARN = ffi.C.S_WARN
@@ -193,6 +196,20 @@ local function verify_static(k, v)
     return true
 end
 
+local function parse_format(log)
+    -- There is no easy way to get pointer to ponter via FFI
+    local str_p = ffi.new('const char*[1]')
+    str_p[0] = ffi.cast('char*', log)
+    local logger_type = ffi.new('enum say_logger_type[1]')
+    local rc = ffi.C.say_parse_logger_type(str_p, logger_type)
+
+    if rc ~= 0 then
+        box.error()
+    end
+
+    return logger_type[0]
+end
+
 -- Test if format is valid.
 local function verify_format(key, name, cfg)
     assert(log_cfg[key] ~= nil)
@@ -213,9 +230,7 @@ local function verify_format(key, name, cfg)
     -- The good thing that we're only needed log
     -- entry which is the same key for both.
     if cfg ~= nil and cfg['log'] ~= nil then
-        if string.startswith(cfg['log'], "syslog:") then
-            log_type = ffi.C.SAY_LOGGER_SYSLOG
-        end
+        log_type = parse_format(cfg['log'])
     end
 
     if fmt_str2num[name] == ffi.C.SF_JSON then
@@ -465,7 +480,7 @@ local function load_cfg(self, cfg)
     -- log option might be zero length string, which
     -- is fine, we should treat it as nil.
     if cfg.log ~= nil then
-        if type(cfg.log) == 'string' and cfg.log:len() == 0 then
+        if type(cfg.log) ~= 'string' or cfg.log:len() == 0 then
             cfg.log = nil
         end
     end
@@ -511,6 +526,11 @@ local function load_cfg(self, cfg)
         nonblock = 1
     end
 
+    -- Parsing for validation purposes
+    if cfg.log ~= nil then
+        parse_format(cfg.log)
+    end
+
     -- We never allow confgure the logger in background
     -- mode since we don't know how the box will be configured
     -- later.
diff --git a/test/app-tap/gh-5130-panic-on-invalid-log.test.lua b/test/app-tap/gh-5130-panic-on-invalid-log.test.lua
new file mode 100755
index 000000000..24af8139c
--- /dev/null
+++ b/test/app-tap/gh-5130-panic-on-invalid-log.test.lua
@@ -0,0 +1,23 @@
+#!/usr/bin/env tarantool
+
+local test = require('tap').test('gh-5130')
+local log = require('log')
+test:plan(3)
+
+--
+-- gh-5130: panic on invalid "log"
+--
+
+-- Invalid log. No panic, just error
+_, err = pcall(log.cfg, {log=' :invalid'})
+test:like(err, "expecting a file name or a prefix")
+
+-- Empty string - default log (to be compatible with box.cfg)
+ok = pcall(log.cfg, {log=''})
+test:ok(ok)
+
+-- Dynamic reconfiguration - error, no info about invalid logger type
+_, err = pcall(log.cfg, {log=' :invalid'})
+test:like(err, "can't be set dynamically")
+
+os.exit(test:check() and 0 or 1)
-- 
2.23.0

Re: [Tarantool-patches] [PATCH] lua: fix panic in case when log.cfg.log incorrecly specified

[Cyrill Gorcunov]

On Wed, Sep 09, 2020 at 11:19:55AM +0300, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> This patch makes log.cfg{log = ...} behaviour the same as in
> box.cfg{log = ...} and fixes panic if "log" is incorrectly
> specified. For such purpose we export "say_parse_logger_type"
> function and use for logger type validation and logger type
> parsing.
> 
> Closes #5130
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>

Thanks!

Re: [Tarantool-patches] [PATCH] lua: fix panic in case when log.cfg.log incorrecly specified

[Kirill Yukhin]

Hello,

On 09 сен 11:19, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> This patch makes log.cfg{log = ...} behaviour the same as in
> box.cfg{log = ...} and fixes panic if "log" is incorrectly
> specified. For such purpose we export "say_parse_logger_type"
> function and use for logger type validation and logger type
> parsing.
> 
> Closes #5130
> ---
> Issue: https://github.com/tarantool/tarantool/issues/5130 
> Branch: https://github.com/tarantool/tarantool/tree/olegrok/5130-panic-on-invalid-log

I've checked your patch into 2.5 and master.

--
Regards, Kirill Yukhin