Re: [Tarantool-patches] [PATCH 1/1] app: os.setenv() affects os.environ()

[Igor Munkin <imun@tarantool.org>]

Vlad,

Thanks for the patch, LGTM.

Minor: I see no particular reason to implement the test below in "diff"
instead of TAP, but I don't insist since all other related tests are
already implemented in "diff".

Side note: OMG, I guess spoiling Lua standart namespaces is some kind of
obsession. I can understand the reason of adding new methods in string
table since it's a metatable for all string objects. However, I can't
understand this activity regarding other standart namespaces starting
from Mike with his table.new (but thanks for an additional 'require'
required to enabled this feature) and finishing with #1718. I hope
sometime we move everything to a separate namespace like the one uJIT
has. The remark is not related to the patch, the changes are good,
thanks again.

On 06.02.20, Vladislav Shpilevoy wrote:
> os.setenv() and os.environ() are Lua API for
> 
>     extern char **environ;
>     int setenv();
> 
> The Open Group standardized access points for environment
> variables. But there is no a word about that environ never
> changes. Programs can't relay on that. For example, addition of
> a new variable may cause realloc of the whole environ array, and
> therefore change of its pointer value. That was exactly the case
> in os.environ() - it was using value of environ array remembered
> when Tarantool started.
> 
> And os.setenv() could realloc the array and turn the saved pointer
> into garbage.
> 
> Closes #4733
> ---
> Issue: https://github.com/tarantool/tarantool/issues/4733
> Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4733-os.environ
> 
>  src/lua/env.lua       |  3 +--
>  test/app/env.result   | 46 +++++++++++++++++++++++++++++++++++++++----
>  test/app/env.test.lua | 27 +++++++++++++++++++++----
>  3 files changed, 66 insertions(+), 10 deletions(-)
> 
> diff --git a/src/lua/env.lua b/src/lua/env.lua
> index b150a48a9..dd1616a84 100644
> --- a/src/lua/env.lua
> +++ b/src/lua/env.lua
> @@ -9,9 +9,8 @@ ffi.cdef[[
>      int   unsetenv(const char *name);
>  ]]
>  
> -local environ = ffi.C.environ
> -
>  os.environ = function()
> +    local environ = ffi.C.environ
>      if not environ then
>          return nil
>      end
> diff --git a/test/app/env.result b/test/app/env.result
> index f725b01c0..c584947fb 100644
> --- a/test/app/env.result
> +++ b/test/app/env.result
> @@ -24,6 +24,9 @@ type(env_dict)
>  ---
>  - table
>  ...
> +err = nil
> +---
> +...
>  test_run:cmd("setopt delimiter ';'")
>  ---
>  - true
> @@ -31,11 +34,46 @@ test_run:cmd("setopt delimiter ';'")
>  do
>      for k, v in pairs(env_dict) do
>          if type(k) ~= 'string' or type(v) ~= 'string' then
> -            return false
> +            err = {k, v}
> +            break
>          end
>      end
> -    return true
> -end;
> +end
> +test_run:cmd("setopt delimiter ''");
>  ---
> -- true
> +...
> +err
> +---
> +- null
> +...
> +--
> +-- gh-4733: os.setenv() should affect os.environ
> +--
> +size = 0
> +---
> +...
> +for _, __ in pairs(os.environ()) do size = size + 1 end
> +---
> +...
> +for i = 1, size do os.setenv('gh-4733-test-var'..i, tostring(i)) end
> +---
> +...
> +env = os.environ()
> +---
> +...
> +err = nil
> +---
> +...
> +for i = 1, size do                                                  \
> +    local value = env['gh-4733-test-var'..i]                        \
> +    if value ~= tostring(i) then                                    \
> +        err = {i, value}                                            \
> +        break                                                       \
> +    end                                                             \
> +end
> +---
> +...
> +err
> +---
> +- null
>  ...
> diff --git a/test/app/env.test.lua b/test/app/env.test.lua
> index fbc1ace86..b725f0124 100644
> --- a/test/app/env.test.lua
> +++ b/test/app/env.test.lua
> @@ -9,13 +9,32 @@ do os.getenv('location') end
>  
>  env_dict = os.environ()
>  type(env_dict)
> +err = nil
>  test_run:cmd("setopt delimiter ';'")
>  do
>      for k, v in pairs(env_dict) do
>          if type(k) ~= 'string' or type(v) ~= 'string' then
> -            return false
> +            err = {k, v}
> +            break
>          end
>      end
> -    return true
> -end;
> -test_run:cmd("setopt delimiter ''")
> +end
> +test_run:cmd("setopt delimiter ''");
> +err
> +
> +--
> +-- gh-4733: os.setenv() should affect os.environ
> +--
> +size = 0
> +for _, __ in pairs(os.environ()) do size = size + 1 end
> +for i = 1, size do os.setenv('gh-4733-test-var'..i, tostring(i)) end
> +env = os.environ()
> +err = nil
> +for i = 1, size do                                                  \
> +    local value = env['gh-4733-test-var'..i]                        \
> +    if value ~= tostring(i) then                                    \
> +        err = {i, value}                                            \
> +        break                                                       \
> +    end                                                             \
> +end
> +err
> -- 
> 2.21.1 (Apple Git-122.3)
> 

-- 
Best regards,
IM