Skip to content

Commit 2284dc8

Browse files
timmc-edxUsamaSadiq
authored andcommitted
fix: Don't let local codejail exec pollute darklaunched remote globals
We were running local exec before making the copy of globals_dict for remote_exec, so remote exec has been getting a polluted version of the globals.
1 parent 8db188e commit 2284dc8

File tree

2 files changed

+22
-2
lines changed

2 files changed

+22
-2
lines changed

xmodule/capa/safe_exec/safe_exec.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,11 @@ def safe_exec(
180180

181181
else:
182182

183+
# Create a copy so the originals are not modified as part of this call.
184+
# This has to happen before local exec is run, since globals are modified
185+
# as a side effect.
186+
darklaunch_globals = copy.deepcopy(globals_dict)
187+
183188
# Decide which code executor to use.
184189
if unsafely:
185190
exec_fn = codejail_not_safe_exec
@@ -215,8 +220,6 @@ def safe_exec(
215220
# when in darklaunch mode.
216221
if is_codejail_in_darklaunch():
217222
try:
218-
# Create a copy so the originals are not modified as part of this call.
219-
darklaunch_globals = copy.deepcopy(globals_dict)
220223
data = {
221224
"code": code_prolog + LAZY_IMPORTS + code,
222225
"globals_dict": darklaunch_globals,

xmodule/capa/safe_exec/tests/test_safe_exec.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
"""Test safe_exec.py"""
22

33

4+
import copy
45
import hashlib
56
import os
67
import os.path
@@ -216,10 +217,21 @@ def test_separate_globals(self):
216217
# Both will attempt to read and write the 'overwrite' key.
217218
globals_dict = {'overwrite': 'original'}
218219

220+
local_globals = None
221+
remote_globals = None
222+
219223
def local_exec(code, globals_dict, **kwargs):
224+
# Preserve what local exec saw
225+
nonlocal local_globals
226+
local_globals = copy.deepcopy(globals_dict)
227+
220228
globals_dict['overwrite'] = 'mock local'
221229

222230
def remote_exec(data):
231+
# Preserve what remote exec saw
232+
nonlocal remote_globals
233+
remote_globals = copy.deepcopy(data['globals_dict'])
234+
223235
data['globals_dict']['overwrite'] = 'mock remote'
224236
return (None, None)
225237

@@ -245,6 +257,11 @@ def remote_exec(data):
245257
)
246258
assert results['raised'] is None
247259

260+
# Both arms should have only seen the original globals object, untouched
261+
# by the other arm.
262+
assert local_globals == {'overwrite': 'original'}
263+
assert remote_globals == {'overwrite': 'original'}
264+
248265
def test_remote_runs_even_if_local_raises(self):
249266
"""Test that remote exec runs even if local raises."""
250267
def local_exec(code, globals_dict, **kwargs):

0 commit comments

Comments
 (0)