[Tarantool-patches] [PATCH v2 0/2] protects box.cfg from raw data modification

[Tarantool-patches] [PATCH v2 0/2] protects box.cfg from raw data modification

[Olga Arkhangelskaia]

Patch set enables DLUAJIT_ENABLE_PAIRSMM flag by default and protects
box.cfg from raw data modification. 

Branch: https://github.com/tarantool/tarantool/tree/OKriw/gh-2867-raise-on-raw-modifications-of-box.cfg-values
v1:https://lists.tarantool.org/pipermail/tarantool-patches/2019-November/012316.html

Changes in v2:                                                                  
- deleted second test                                                           
- deleted unnecessary variables                                                 
- changed subject/commit msg 

Olga Arkhangelskaia (2):
  build: enables DLUAJIT_ENABLE_PAIRSMM by default
  box: protects box.cfg from raw data modification

 cmake/luajit.cmake        |  1 +
 src/box/lua/load_cfg.lua  | 14 +++++++++++++-
 test/box-tap/cfg.test.lua | 11 ++++++++++-
 3 files changed, 24 insertions(+), 2 deletions(-)

-- 
2.20.1 (Apple Git-117)

[Tarantool-patches] [PATCH v2 1/2] build: enables DLUAJIT_ENABLE_PAIRSMM by default

[Olga Arkhangelskaia]

Turns on DLUAJIT_ENABLE_PAIRSMM flag for tarantool build.
Now pairs/ipairs metamehods are available.
---
 cmake/luajit.cmake | 1 +
 1 file changed, 1 insertion(+)

diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
index 10df633d5..072db8269 100644
--- a/cmake/luajit.cmake
+++ b/cmake/luajit.cmake
@@ -217,6 +217,7 @@ macro(luajit_build)
         add_definitions(-DLUAJIT_USE_ASAN=1)
         set (luajit_ldflags ${luajit_ldflags} -fsanitize=address)
     endif()
+    add_definitions(-DLUAJIT_ENABLE_PAIRSMM=1)
     add_definitions(-DLUAJIT_SMART_STRINGS=1)
     # Add all COMPILE_DEFINITIONS to xcflags
     get_property(defs DIRECTORY PROPERTY COMPILE_DEFINITIONS)
-- 
2.20.1 (Apple Git-117)

Re: [Tarantool-patches] [PATCH v2 1/2] build: enables DLUAJIT_ENABLE_PAIRSMM by default

[Igor Munkin]

Olya,

Thanks for the patch, LGTM.

On 22.11.19, Olga Arkhangelskaia wrote:
> Turns on DLUAJIT_ENABLE_PAIRSMM flag for tarantool build.
> Now pairs/ipairs metamehods are available.
> ---
>  cmake/luajit.cmake | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> index 10df633d5..072db8269 100644
> --- a/cmake/luajit.cmake
> +++ b/cmake/luajit.cmake
> @@ -217,6 +217,7 @@ macro(luajit_build)
>          add_definitions(-DLUAJIT_USE_ASAN=1)
>          set (luajit_ldflags ${luajit_ldflags} -fsanitize=address)
>      endif()
> +    add_definitions(-DLUAJIT_ENABLE_PAIRSMM=1)
>      add_definitions(-DLUAJIT_SMART_STRINGS=1)
>      # Add all COMPILE_DEFINITIONS to xcflags
>      get_property(defs DIRECTORY PROPERTY COMPILE_DEFINITIONS)
> -- 
> 2.20.1 (Apple Git-117)
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 1/2] build: enables DLUAJIT_ENABLE_PAIRSMM by default

[Igor Munkin]

Olya,

On the second thought, I see this patch delivers exactly the feature
requested in #4560[1]. Please consider to separate it from the provided
patchset and rebase the second patch on it.

Besides, do not forget to append a magic line to the commit message:
| Closes #4560

[1]: https://github.com/tarantool/tarantool/issues/4560

-- 
Best regards,
IM

[Tarantool-patches] [PATCH v2 2/2] box: protects box.cfg from raw data modification

[Olga Arkhangelskaia]

Forbids the possibility of the raw modification for box.cfg table.
Now the only way to change table value is box.cfg{}.

Closes #2867
---
 src/box/lua/load_cfg.lua  | 14 +++++++++++++-
 test/box-tap/cfg.test.lua | 11 ++++++++++-
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 85617c8f0..27ac6bb77 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -533,12 +533,24 @@ local function load_cfg(cfg)
     end
     setmetatable(box, nil)
     box_configured = nil
-    box.cfg = setmetatable(cfg,
+
+    local actual = cfg
+    box.cfg = setmetatable({},
         {
             __newindex = function(table, index)
                 error('Attempt to modify a read-only table')
             end,
             __call = locked(reload_cfg),
+            __index = function (self, k)
+                return actual[k]
+            end,
+            __serialize = function() return actual end,
+            __pairs = function(self)
+                local function iter(actual, k)
+                    return next(actual, k)
+                end
+                return iter, actual, nil
+            end
         })
     private.cfg_load()
     for key, fun in pairs(dynamic_cfg) do
diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
index d529447bb..443dfafbc 100755
--- a/test/box-tap/cfg.test.lua
+++ b/test/box-tap/cfg.test.lua
@@ -6,7 +6,7 @@ local socket = require('socket')
 local fio = require('fio')
 local uuid = require('uuid')
 local msgpack = require('msgpack')
-test:plan(104)
+test:plan(105)
 
 --------------------------------------------------------------------------------
 -- Invalid values
@@ -592,6 +592,15 @@ box.cfg{read_only=true}
 ]]
 test:is(run_script(code), PANIC, "panic on bootstrapping a read-only instance as master")
 
+--
+-- gf-2867 raise on raw modifications of box.cfg values
+--
+code = [[
+box.cfg{}
+box.cfg["read_only"] = true
+]]
+
+test:is(run_script(code), PANIC, "attempt to modify a read-only table")
 
 test:check()
 os.exit(0)
-- 
2.20.1 (Apple Git-117)

Re: [Tarantool-patches] [PATCH v2 2/2] box: protects box.cfg from raw data modification

[Igor Munkin]

Olya,

Thanks for the patch, it looks similar to the one proposed by Mons long
time ago. However, as a result of the offline discussion with him, we
faced the fact that the provided patch still allows a modification of
nested objects, e.g. replication. Consider the following:
| $ ./src/tarantool
| Tarantool 2.2.1-114-g6c8acacef
| type 'help' for interactive help
| tarantool> box.cfg{ replication_connect_quorum=0, replication={"127.0.0.1:3301","127.0.0.2:3301"} }
| 2019-11-22 17:51:14.861 [29536] main/102/interactive C> Tarantool 2.2.1-114-g6c8acacef
| 2019-11-22 17:51:14.861 [29536] main/102/interactive C> log level 5
| 2019-11-22 17:51:14.861 [29536] main/102/interactive I> mapping 268435456 bytes for memtx tuple arena...
| 2019-11-22 17:51:14.862 [29536] main/102/interactive I> mapping 134217728 bytes for vinyl tuple arena...
| 2019-11-22 17:51:14.876 [29536] main/102/interactive I> instance uuid 78b17737-e0bd-4773-885e-bb15fbecc406
| 2019-11-22 17:51:14.876 [29536] main/102/interactive I> connecting to 2 replicas
| <snip>
| 2019-11-22 17:51:44.935 [29536] snapshot/101/main I> done
| 2019-11-22 17:51:44.937 [29536] main/102/interactive I> ready to accept requests
| 2019-11-22 17:51:44.937 [29536] main/104/checkpoint_daemon I> scheduled next checkpoint for Fri Nov 22 19:08:25 2019
| 2019-11-22 17:51:44.938 [29536] main/102/interactive I> set 'replication_connect_quorum' configuration option to 0
| 2019-11-22 17:51:44.938 [29536] main/102/interactive I> set 'replication' configuration option to ["127.0.0.1:3301","127.0.0.2:3301"]
| ---
| ...
| tarantool> box.cfg
| ---
| - vinyl_run_count_per_level: 2
|   <snip>
|   replication:
|   - 127.0.0.1:3301
|   - 127.0.0.2:3301
|   <snip>
| ...
|
| tarantool> box.cfg.replication = nil
| ---
| - error: 'builtin/box/load_cfg.lua:541: Attempt to modify a read-only table'
| ...
|
| tarantool> box.cfg.replication[1] = 'QQ'
| ---
| ...
|
| tarantool> box.cfg
| ---
| - vinyl_run_count_per_level: 2
|   <snip>
|   replication:
|   - QQ
|   - 127.0.0.2:3301
|   <snip>
| ...
Thereby some fields in box.cfg are still mutable after the patch.

Besides, I left some comments below related to the test you attached to
the patch. Please consider them too.

On 22.11.19, Olga Arkhangelskaia wrote:
> Forbids the possibility of the raw modification for box.cfg table.
> Now the only way to change table value is box.cfg{}.
> 
> Closes #2867
> ---
>  src/box/lua/load_cfg.lua  | 14 +++++++++++++-
>  test/box-tap/cfg.test.lua | 11 ++++++++++-
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 85617c8f0..27ac6bb77 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -533,12 +533,24 @@ local function load_cfg(cfg)
>      end
>      setmetatable(box, nil)
>      box_configured = nil
> -    box.cfg = setmetatable(cfg,
> +
> +    local actual = cfg
> +    box.cfg = setmetatable({},
>          {
>              __newindex = function(table, index)
>                  error('Attempt to modify a read-only table')
>              end,
>              __call = locked(reload_cfg),
> +            __index = function (self, k)
> +                return actual[k]
> +            end,
> +            __serialize = function() return actual end,
> +            __pairs = function(self)
> +                local function iter(actual, k)
> +                    return next(actual, k)
> +                end
> +                return iter, actual, nil
> +            end
>          })
>      private.cfg_load()
>      for key, fun in pairs(dynamic_cfg) do
> diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
> index d529447bb..443dfafbc 100755
> --- a/test/box-tap/cfg.test.lua
> +++ b/test/box-tap/cfg.test.lua
> @@ -6,7 +6,7 @@ local socket = require('socket')
>  local fio = require('fio')
>  local uuid = require('uuid')
>  local msgpack = require('msgpack')
> -test:plan(104)
> +test:plan(105)
>  
>  --------------------------------------------------------------------------------
>  -- Invalid values
> @@ -592,6 +592,15 @@ box.cfg{read_only=true}
>  ]]
>  test:is(run_script(code), PANIC, "panic on bootstrapping a read-only instance as master")
>  
> +--
> +-- gf-2867 raise on raw modifications of box.cfg values
> +--
> +code = [[
> +box.cfg{}
> +box.cfg["read_only"] = true
> +]]
> +
> +test:is(run_script(code), PANIC, "attempt to modify a read-only table")

The provided test case is fine, but I see we can extend it with
following checks:
* check the one can't add new values to box.cfg, e.g. a QQ
* check that pairs iterator yields the correct values
* consider adding a separate case related to replication table
  modification

>  
>  test:check()
>  os.exit(0)
> -- 
> 2.20.1 (Apple Git-117)
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 0/2] protects box.cfg from raw data modification

[Igor Munkin]

Olya,

I reviewed both patches within this set and left some comments. Please
consider to rebase you branch on 5d2105b that introduces __pairs
handling to our LuaJIT fork.

On 22.11.19, Olga Arkhangelskaia wrote:
> Patch set enables DLUAJIT_ENABLE_PAIRSMM flag by default and protects
> box.cfg from raw data modification. 
> 
> Branch: https://github.com/tarantool/tarantool/tree/OKriw/gh-2867-raise-on-raw-modifications-of-box.cfg-values
> v1:https://lists.tarantool.org/pipermail/tarantool-patches/2019-November/012316.html
> 
> Changes in v2:                                                                  
> - deleted second test                                                           
> - deleted unnecessary variables                                                 
> - changed subject/commit msg 
> 
> Olga Arkhangelskaia (2):
>   build: enables DLUAJIT_ENABLE_PAIRSMM by default
>   box: protects box.cfg from raw data modification
> 
>  cmake/luajit.cmake        |  1 +
>  src/box/lua/load_cfg.lua  | 14 +++++++++++++-
>  test/box-tap/cfg.test.lua | 11 ++++++++++-
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> -- 
> 2.20.1 (Apple Git-117)
> 

-- 
Best regards,
IM