Issue 3037 scripts must not share globals #3038

htgoebel · 2017-11-25T12:47:34Z

This is an attempt to solve #3037. This moves the creation of the __main__ module into the loop and removes this module from sys.modules after the script has been executed.

htgoebel · 2017-11-25T12:48:02Z

@codewarrior0
The resp. tests currently fails with this error message. Any idea what may cause this?

$ pytest tests/functional/test_basic.py -k test_several_scripts2 -vs
[…]
RUNNING:  '…/test_several_scripts20/dist/several-scripts2/several-scripts2' , args:  ['./several-scripts2']
Traceback (most recent call last):
  File "specs/several-scripts/rt-hook-script.py", line 1, in <module>
ImportError: __import__ not found

This script is the second one (the first one is pyiboot01_bootstrap):

(174, 1137, 2578, 1, 'm', u'pyimod01_os_path'),
 (1311, 4339, 11963, 1, 'm', u'pyimod02_archive'),
 (5650, 7365, 22425, 1, 'm', u'pyimod03_importers'),
 (13015, 1817, 5039, 1, 's', u'pyiboot01_bootstrap'),
 (14832, 336, 621, 1, 's', u'rt-hook-script'),

So it looks like the module is not properly set up by PyImport_AddModule("__main__"), even if the module has been removed from´sys.modules`.

This will ensure each scripts will get its own global variables. - Move the creation of the '__main__' module into the loop and remove this module from sys.modules after the script has been executed. - Use PyImport_ExecCodeModule() for executing the scripts. - No longer set the __file__ attribute (now done by PyImport_ExecCodeModule). Using PyImport_AddModule within the loop did not work: For the second script, '__builtins__' have not been set, so even a simple import failed. (See pull-request pyinstaller#3038 for details.)

htgoebel · 2017-11-25T14:56:19Z

Updated the pull-request. What I learned meanwhile:

The test passes (will, the one I tried) when adding import sys ; del sys.modules['__main__'] to the end of rt-hook-script.py, and removing the resp. code from the bootloader. But doing the same in the bootloader (using PyRun_SimpleString()) does not work. __builtins__ have not been defined anymore.

So I switched to a higher level import function (PyImport_ExecCodeModule()), which handles this case and takes care of some other things, too. Using this function test passes (well, the one I tried).

After finishing this work, I discovered that test_several_scripts2 passes, while test_several_scripts1 still fails. No.2 is just checking if the a variable set be a former script is in the later script's scope. No. 1 is based on the original issue (using super()). So there seems to be more going on.

Ups, and some other tests fail, too. :-(

This will ensure each scripts will get its own global variables. - Move the creation of the '__main__' module into the loop and remove this module from sys.modules after the script has been executed. - Use PyImport_ExecCodeModule() for executing the scripts. - No longer set the __file__ attribute (now done by PyImport_ExecCodeModule). Using PyImport_AddModule within the loop did not work: For the second script, '__builtins__' have not been set, so even a simple import failed. (See pull-request pyinstaller#3038 for details.)

This is required since now the bootloader doesn't set the `__file__` attribute from the archive name anymore, but leaves this to `PyImport_ExecCodeModule`, which looks at code.co_filename. Hopefully this will not give us another unicode error.

Given the latest changes, calculating the filename is no longer necessary. Remove it and change the `VS()` call, so the output keeps the same.

Update wording and move to better place.

This was unnoticed since while all scripts shared the same global variables since the import was done by some bootstrap script running before.

Given the latest changes, pyiboot01_bootstrap's globals are gone (more precisely None) after the script has finished. But the hooked classes still reference to them (including 'sys' and 'os'). I decided to make this into a module instead of more complicated code like mix-in-classes and lot of import statements.

htgoebel · 2017-11-25T17:23:17Z

Next iteration: I fixed some other test-cases and found the reason why test_several_scripts1 is failing: It's the same reason the cytpes hooks failed in pyiboot01_bootstrap.py: The script is executed as a script and all globals are gone afterwards. Now if the script hooks into somewhere and the hook tries to access any global variable in the script's scope, this variable will be None.

This will also hit the run-time hooks, so we should think about a different solution.

Maybe the script's module should be given a different name in sys.modules instead of removing it.

Any ideas?

ghost

This seems to be overkill for what is trying to be done.

ghost · 2017-11-30T05:25:21Z

bootloader/src/pyi_launch.c

            if (!code) {
                FATALERROR("Failed to unmarshal code object for %s\n", ptoc->name);
                PI_PyErr_Print();
                return -1;
            }
-            /* Run it */
-            retval = PI_PyEval_EvalCode(code, main_dict, main_dict);


Do we really need to remove this function? Seems like each iteration of the loop can be this:

main_dict_copy = PyDict_Copy(main_dict) Py_INCREF(main_dict_copy) retval = PI_PyEval_EvalCode(code, main_dict_copy, main_dict_copy);

This would be an idea to try. Thanks.

This does not work either :-( Same effect as with my code.

ghost · 2017-11-30T05:44:27Z

bootloader/src/pyi_launch.c

                PI_PyErr_Print();
                /* If the error was SystemExit, PyErr_Print calls exit() without
                 * returning. So don't print "Failed to execute" on SystemExit. */
                FATALERROR("Failed to execute script %s\n", ptoc->name);
                return -1;
            }
+            Py_DECREF(module);


Wait, how can you decref the module and then complain when you can no longer access its attributes?

I would expect the code to reference the dict, too. But anyway, I also tried not decrementing the reference count here with no success. But I'll double-check.

Legorooj · 2020-02-16T07:42:13Z

@htgoebel status on this?

htgoebel · 2020-05-02T09:25:47Z

@Legorooj THis still needs to be solved :-(

Move the ctypes hooks from bootstrap script into separate module (pymod04_ctypes). The main motivation for this is to prevent the modifications to global namespace from affecting the hook ctypes hooks code; speficially, if user decides to use `sys` or `os` as variables in the global namespace, that should not effect the ctype hook function _frozen_name(), which treats `sys` and `os` as names modules (which were bound to those names when the bootstrap script was executing). Fixes pyinstaller#5797. This commit is a cleaned-up and modernized version of an earlier commit from pyinstaller#3038 (daf60a6). Co-authored-by: Hartmut Goebel <h.goebel@crazy-compilers.com>

Move the ctypes hooks from bootstrap script into separate module (pymod04_ctypes). The main motivation for this is to prevent the modifications to global namespace from affecting the ctypes hooks code; specifically, if user decides to use `sys` or `os` as variables in the global namespace, that should not effect the ctype hook function _frozen_name(), which treats `sys` and `os` as names of modules (which were bound to those names when the bootstrap script was executed). Fixes pyinstaller#5797. This commit is a cleaned-up and modernized version of an earlier commit from pyinstaller#3038 (daf60a6). Co-authored-by: Hartmut Goebel <h.goebel@crazy-compilers.com>

Move the ctypes hooks from bootstrap script into separate module (pymod04_ctypes). The main motivation for this is to prevent the modifications to global namespace from affecting the ctypes hooks code. Specifically, if user decides to use `sys` or `os` as variables in the global namespace, that should not effect the ctypes hook function _frozen_name(), which treats `sys` and `os` as names of modules (which were bound to those names when the bootstrap script was executed). Fixes pyinstaller#5797. This commit is a cleaned-up and modernized version of corresponding commit from pyinstaller#3038 (daf60a6). Co-authored-by: Hartmut Goebel <h.goebel@crazy-compilers.com>

Move the ctypes hooks from bootstrap script into separate module (pymod04_ctypes). The main motivation for this is to prevent the modifications to global namespace from affecting the ctypes hooks code. Specifically, if user decides to use `sys` or `os` as variables in the global namespace, that should not effect the ctypes hook function _frozen_name(), which treats `sys` and `os` as names of modules (which were bound to those names when the bootstrap script was executed). Fixes #5797. This commit is a cleaned-up and modernized version of corresponding commit from #3038 (daf60a6). Co-authored-by: Hartmut Goebel <h.goebel@crazy-compilers.com>

Tests: Add test-cases for module '__main__'.

8f5c214

htgoebel requested a review from codewarrior0 November 25, 2017 12:48

htgoebel force-pushed the issue-3037-scripts-must-not-share-globals branch from 2a992ba to 58707f8 Compare November 25, 2017 14:44

htgoebel added 9 commits November 25, 2017 18:04

Tests: Add a comment.

d649976

Tests: Make some tests more verbose in case of an error.

6af0012

Bootloader: Small cleanup.

e982de9

Given the latest changes, calculating the filename is no longer necessary. Remove it and change the `VS()` call, so the output keeps the same.

Bootloader: Update comments.

a713136

Update wording and move to better place.

Tests: Fix missing import in a test-case.

e8f0773

This was unnoticed since while all scripts shared the same global variables since the import was done by some bootstrap script running before.

Tests: Add test-case if ctypes-hooks are in place.

97e200c

htgoebel force-pushed the issue-3037-scripts-must-not-share-globals branch from 58707f8 to 97e200c Compare November 25, 2017 17:13

ghost reviewed Nov 30, 2017

View reviewed changes

htgoebel added the area:bootloader Caused be or effecting the bootloader label Dec 6, 2017

htgoebel added this to the PyInstaller 3.4 milestone Dec 7, 2017

htgoebel added state:needs more work This PR needs more work help wanted Seeking help by somebody with deeper knowledge on this topic. labels Mar 13, 2018

htgoebel modified the milestones: PyInstaller 3.4, PyInstaller 3.x Sep 13, 2018

Legorooj force-pushed the develop branch 3 times, most recently from 81c364b to 3a8eccd Compare April 17, 2021 03:54

Legorooj force-pushed the develop branch from 3a8eccd to e20e74c Compare April 17, 2021 04:06

rokm mentioned this pull request May 20, 2021

loader: move ctypes hooks into a module #5850

Merged

Legorooj force-pushed the develop branch from 1b31d25 to 90431c0 Compare July 13, 2021 10:47

bwoodsend force-pushed the develop branch from edceb66 to 30d949d Compare July 20, 2021 16:01

bwoodsend force-pushed the develop branch from 98913a3 to 51189f6 Compare November 17, 2021 21:55

rokm mentioned this pull request Jun 21, 2023

rthook: harden run-time hooks against global namespace changes #7705

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 3037 scripts must not share globals #3038

Issue 3037 scripts must not share globals #3038

htgoebel commented Nov 25, 2017

htgoebel commented Nov 25, 2017

htgoebel commented Nov 25, 2017

htgoebel commented Nov 25, 2017

ghost left a comment

ghost Nov 30, 2017 •

edited by ghost

htgoebel Nov 30, 2017

htgoebel Dec 1, 2017

ghost Nov 30, 2017

htgoebel Nov 30, 2017

Legorooj commented Feb 16, 2020

htgoebel commented May 2, 2020

Issue 3037 scripts must not share globals #3038

Are you sure you want to change the base?

Issue 3037 scripts must not share globals #3038

Conversation

htgoebel commented Nov 25, 2017

htgoebel commented Nov 25, 2017

htgoebel commented Nov 25, 2017

htgoebel commented Nov 25, 2017

ghost left a comment

Choose a reason for hiding this comment

ghost Nov 30, 2017 • edited by ghost

Choose a reason for hiding this comment

htgoebel Nov 30, 2017

Choose a reason for hiding this comment

htgoebel Dec 1, 2017

Choose a reason for hiding this comment

ghost Nov 30, 2017

Choose a reason for hiding this comment

htgoebel Nov 30, 2017

Choose a reason for hiding this comment

Legorooj commented Feb 16, 2020

htgoebel commented May 2, 2020

ghost Nov 30, 2017 •

edited by ghost