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

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

[Vladislav Shpilevoy]

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)

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

[Igor Munkin]

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

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

[Nikita Pettik]

On 06 Feb 23:47, 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
> ---

LGTM

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

[Vladislav Shpilevoy]

Hi! Thanks all for the reviews!

Pushed to master, 2.3, 2.2 and 1.10.

Also added a changelog record to the releases draft.
And here too, since I forgot to add it to the cover
letter (or that rule was added after my patch, don't
remember).

@ChangeLog
- os.environ() was not changed when os.setenv() was
  called (gh-4733).

On 19/02/2020 22:42, Nikita Pettik wrote:
> On 06 Feb 23:47, 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
>> ---
> 
> LGTM
>