From d3b87552091f3db462b4e6ab8e34816146d16510 Mon Sep 17 00:00:00 2001 From: "R. Garcia-Dias" Date: Thu, 3 Sep 2026 11:11:08 +0100 Subject: [PATCH 1/9] fix(nnunet): harden nnUNetV2Runner against code execution - predict_ensemble_postprocessing: warn before unpickling the postprocessing_file read from inference_information.json (GHSA-8f32-8649-rv87). - train_parallel: run each training command as an argv list with shell=False instead of joining with shlex into a shell=True string, closing the Windows cmd.exe quoting bypass (GHSA-qv7x-wq36-2cm7). Signed-off-by: R. Garcia-Dias --- monai/apps/nnunet/nnunetv2_runner.py | 36 ++++++--- .../nnunet/test_nnunetv2_runner_command.py | 75 +++++++++++++++++++ 2 files changed, 99 insertions(+), 12 deletions(-) diff --git a/monai/apps/nnunet/nnunetv2_runner.py b/monai/apps/nnunet/nnunetv2_runner.py index c18e7bcd05..dc5989c2fa 100644 --- a/monai/apps/nnunet/nnunetv2_runner.py +++ b/monai/apps/nnunet/nnunetv2_runner.py @@ -17,6 +17,7 @@ import re import shlex import subprocess +import warnings from typing import Any import monai @@ -741,17 +742,19 @@ def train_parallel( f"log '.txt' inside '{os.path.join(self.nnunet_results, self.dataset_name)}'" ) for stage in all_cmds: - processes = [] - for device_id in stage: - if not stage[device_id]: - continue - cmd_str = "; ".join(shlex.join(cmd) for cmd, _ in stage[device_id]) - env = stage[device_id][0][1] - logger.info(f"Current running command on GPU device {device_id}:\n{cmd_str}\n") - processes.append(subprocess.Popen(cmd_str, shell=True, env=env, stdout=subprocess.DEVNULL)) - # finish this stage first - for p in processes: - p.wait() + max_cmds_per_gpu = max((len(cmds) for cmds in stage.values()), default=0) + for cmd_index in range(max_cmds_per_gpu): + processes = [] + for device_id, gpu_cmds in stage.items(): + if cmd_index >= len(gpu_cmds): + continue + cmd, env = gpu_cmds[cmd_index] + cmd_str = shlex.join(cmd) + logger.info(f"Current running command on GPU device {device_id}:\n{cmd_str}\n") + processes.append(subprocess.Popen(cmd, shell=False, env=env, stdout=subprocess.DEVNULL)) + # finish this command round on all GPUs before starting the next command + for p in processes: + p.wait() def validate_single_model(self, config: str, fold: int, **kwargs: Any) -> None: """ @@ -996,7 +999,16 @@ def predict_ensemble_postprocessing( # apply postprocessing if run_postprocessing: - pp_fns, pp_fn_kwargs = load_pickle(self.best_configuration["best_model_or_ensemble"]["postprocessing_file"]) + postprocessing_file = self.best_configuration["best_model_or_ensemble"]["postprocessing_file"] + warnings.warn( + f"unpickling postprocessing_file {postprocessing_file}: this path is read from " + "inference_information.json and is loaded with Python pickle without any allow list, " + "which gives whoever controls that file arbitrary code execution. Only proceed if the " + "inference_information.json is from a source you trust " + "(see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-8f32-8649-rv87).", + stacklevel=2, + ) + pp_fns, pp_fn_kwargs = load_pickle(postprocessing_file) apply_postprocessing_to_folder( folder_for_pp, join(target_dir_base, "ensemble_predictions_postprocessed"), diff --git a/tests/apps/nnunet/test_nnunetv2_runner_command.py b/tests/apps/nnunet/test_nnunetv2_runner_command.py index 506c30fad0..3e5379389c 100644 --- a/tests/apps/nnunet/test_nnunetv2_runner_command.py +++ b/tests/apps/nnunet/test_nnunetv2_runner_command.py @@ -11,10 +11,16 @@ from __future__ import annotations +import os +import sys +import types import unittest +import warnings from unittest import mock +from monai.apps.nnunet import nnunetv2_runner from monai.apps.nnunet.nnunetv2_runner import nnUNetV2Runner +from monai.bundle import ConfigParser def _make_runner(export_validation_probabilities=False): @@ -74,5 +80,74 @@ def test_validate_emits_bare_val_flag(self): self.assertNotIn("True", cmd) +class TestTrainParallelCommand(unittest.TestCase): + def test_train_parallel_uses_argv_list_without_shell(self): + runner = _make_runner() + runner.dataset_name = "Dataset001_Test" + runner.nnunet_results = "/tmp/nnunet_results" + + all_cmds = [ + { + 0: [ + (["python", "-m", "train", "--fold", "0"], {"CUDA_VISIBLE_DEVICES": "0"}), + (["python", "-m", "train", "--fold", "1"], {"CUDA_VISIBLE_DEVICES": "0"}), + ], + 1: [(["python", "-m", "train", "--fold", "2"], {"CUDA_VISIBLE_DEVICES": "1"})], + } + ] + + with mock.patch.object(runner, "train_parallel_cmd", return_value=all_cmds): + with mock.patch("monai.apps.nnunet.nnunetv2_runner.subprocess.Popen") as popen: + popen.return_value.wait.return_value = None + runner.train_parallel() + + self.assertEqual(popen.call_count, 3) + for call in popen.call_args_list: + self.assertIsInstance(call.args[0], list) + self.assertFalse(call.kwargs["shell"]) + + +class TestPredictEnsemblePostprocessingWarnings(unittest.TestCase): + def test_postprocessing_pickle_warns_on_untrusted_file(self): + runner = _make_runner() + runner.dataset_name = "Dataset001_Test" + runner.nnunet_raw = "/tmp/nnunet_raw" + runner.nnunet_results = "/tmp/nnunet_results" + runner.best_configuration = { + "best_model_or_ensemble": { + "selected_model_or_models": [{"configuration": "3d_fullres"}], + "postprocessing_file": "/tmp/attacker_controlled_postprocessing.pkl", + "some_plans_file": "/tmp/plans.json", + } + } + + ensemble_mod = types.ModuleType("nnunetv2.ensembling.ensemble") + ensemble_mod.ensemble_folders = mock.MagicMock() + pp_mod = types.ModuleType("nnunetv2.postprocessing.remove_connected_components") + pp_mod.apply_postprocessing_to_folder = mock.MagicMock() + fp_mod = types.ModuleType("nnunetv2.utilities.file_path_utilities") + fp_mod.get_output_folder = mock.MagicMock(return_value="/tmp/model_folder") + + fake_modules = { + "nnunetv2.ensembling.ensemble": ensemble_mod, + "nnunetv2.postprocessing.remove_connected_components": pp_mod, + "nnunetv2.utilities.file_path_utilities": fp_mod, + } + + load_pickle = mock.MagicMock(return_value=([], {})) + with mock.patch.dict(sys.modules, fake_modules): + with mock.patch.object(ConfigParser, "load_config_file", return_value=runner.best_configuration): + with mock.patch.object(nnunetv2_runner, "join", os.path.join): + with mock.patch.object(nnunetv2_runner, "load_pickle", load_pickle): + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + runner.predict_ensemble_postprocessing( + run_predict=False, run_ensemble=False, run_postprocessing=True + ) + + load_pickle.assert_called_once_with("/tmp/attacker_controlled_postprocessing.pkl") + self.assertTrue(any("unpickling postprocessing_file" in str(item.message) for item in caught)) + + if __name__ == "__main__": unittest.main() From a204c8c230464beece5e9df02c2ae83aa0adf690 Mon Sep 17 00:00:00 2001 From: "R. Garcia-Dias" Date: Thu, 3 Sep 2026 14:55:27 +0100 Subject: [PATCH 2/9] fix: address PR #9086 review feedback - train_parallel: run each GPU's commands sequentially in its own thread while devices run in parallel, preserving the original per-device ordering without shell=True or a cross-device round barrier - document the execution and wait behavior in the docstring Signed-off-by: R. Garcia-Dias --- monai/apps/nnunet/nnunetv2_runner.py | 29 ++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/monai/apps/nnunet/nnunetv2_runner.py b/monai/apps/nnunet/nnunetv2_runner.py index dc5989c2fa..6b33ec7a3e 100644 --- a/monai/apps/nnunet/nnunetv2_runner.py +++ b/monai/apps/nnunet/nnunetv2_runner.py @@ -18,6 +18,7 @@ import shlex import subprocess import warnings +from concurrent.futures import ThreadPoolExecutor from typing import Any import monai @@ -711,7 +712,11 @@ def train_parallel( **kwargs: Any, ) -> None: """ - Create the line command for subprocess call for parallel training. + Launch subprocesses for parallel training. + + The commands for each GPU run sequentially on that device, while different devices run in + parallel. Each stage waits for all of its devices to finish before the next stage starts. + Note: to set the number of GPUs to use, use ``gpu_id_for_all`` instead of the `CUDA_VISIBLE_DEVICES` environment variable. @@ -742,19 +747,19 @@ def train_parallel( f"log '.txt' inside '{os.path.join(self.nnunet_results, self.dataset_name)}'" ) for stage in all_cmds: - max_cmds_per_gpu = max((len(cmds) for cmds in stage.values()), default=0) - for cmd_index in range(max_cmds_per_gpu): - processes = [] - for device_id, gpu_cmds in stage.items(): - if cmd_index >= len(gpu_cmds): - continue - cmd, env = gpu_cmds[cmd_index] + device_cmds = [(device_id, gpu_cmds) for device_id, gpu_cmds in stage.items() if gpu_cmds] + if not device_cmds: + continue + + def _run_device_commands(item): + device_id, gpu_cmds = item + for cmd, env in gpu_cmds: cmd_str = shlex.join(cmd) logger.info(f"Current running command on GPU device {device_id}:\n{cmd_str}\n") - processes.append(subprocess.Popen(cmd, shell=False, env=env, stdout=subprocess.DEVNULL)) - # finish this command round on all GPUs before starting the next command - for p in processes: - p.wait() + subprocess.Popen(cmd, shell=False, env=env, stdout=subprocess.DEVNULL).wait() + + with ThreadPoolExecutor(max_workers=len(device_cmds)) as executor: + list(executor.map(_run_device_commands, device_cmds)) def validate_single_model(self, config: str, fold: int, **kwargs: Any) -> None: """ From f354b2b26f67105874521880e4785be82042dd0c Mon Sep 17 00:00:00 2001 From: "R. Garcia-Dias" Date: Thu, 3 Sep 2026 15:02:28 +0100 Subject: [PATCH 3/9] test(nnunet): assert commands run sequentially per device Add a deterministic regression test that train_parallel waits for each command to finish before starting the next on the same device. Signed-off-by: R. Garcia-Dias --- .../nnunet/test_nnunetv2_runner_command.py | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/apps/nnunet/test_nnunetv2_runner_command.py b/tests/apps/nnunet/test_nnunetv2_runner_command.py index 3e5379389c..402900dc34 100644 --- a/tests/apps/nnunet/test_nnunetv2_runner_command.py +++ b/tests/apps/nnunet/test_nnunetv2_runner_command.py @@ -13,6 +13,7 @@ import os import sys +import threading import types import unittest import warnings @@ -106,6 +107,51 @@ def test_train_parallel_uses_argv_list_without_shell(self): self.assertIsInstance(call.args[0], list) self.assertFalse(call.kwargs["shell"]) + def test_commands_run_sequentially_per_device(self): + runner = _make_runner() + runner.dataset_name = "Dataset001_Test" + runner.nnunet_results = "/tmp/nnunet_results" + + all_cmds = [ + { + 0: [ + (["python", "-m", "train", "--fold", "0"], {}), + (["python", "-m", "train", "--fold", "1"], {}), + ] + } + ] + + events = [] + lock = threading.Lock() + + class _FakeProcess: + def __init__(self, cmd): + self.cmd = cmd + + def wait(self): + with lock: + events.append(("wait", self.cmd)) + return 0 + + def _fake_popen(cmd, *args, **kwargs): + with lock: + events.append(("popen", cmd)) + return _FakeProcess(cmd) + + with mock.patch.object(runner, "train_parallel_cmd", return_value=all_cmds): + with mock.patch("monai.apps.nnunet.nnunetv2_runner.subprocess.Popen", side_effect=_fake_popen): + runner.train_parallel() + + self.assertEqual( + events, + [ + ("popen", ["python", "-m", "train", "--fold", "0"]), + ("wait", ["python", "-m", "train", "--fold", "0"]), + ("popen", ["python", "-m", "train", "--fold", "1"]), + ("wait", ["python", "-m", "train", "--fold", "1"]), + ], + ) + class TestPredictEnsemblePostprocessingWarnings(unittest.TestCase): def test_postprocessing_pickle_warns_on_untrusted_file(self): From 09dea08229d7fac34d1b2b7144f09d7122ce0447 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:03:22 +0000 Subject: [PATCH 4/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/apps/nnunet/test_nnunetv2_runner_command.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/tests/apps/nnunet/test_nnunetv2_runner_command.py b/tests/apps/nnunet/test_nnunetv2_runner_command.py index 402900dc34..079005ade9 100644 --- a/tests/apps/nnunet/test_nnunetv2_runner_command.py +++ b/tests/apps/nnunet/test_nnunetv2_runner_command.py @@ -113,12 +113,7 @@ def test_commands_run_sequentially_per_device(self): runner.nnunet_results = "/tmp/nnunet_results" all_cmds = [ - { - 0: [ - (["python", "-m", "train", "--fold", "0"], {}), - (["python", "-m", "train", "--fold", "1"], {}), - ] - } + {0: [(["python", "-m", "train", "--fold", "0"], {}), (["python", "-m", "train", "--fold", "1"], {})]} ] events = [] From 224b4073af38bff5954e6c499f7d71127949a0ed Mon Sep 17 00:00:00 2001 From: "R. Garcia-Dias" Date: Thu, 3 Sep 2026 15:16:43 +0100 Subject: [PATCH 5/9] chore: re-trigger CI From e6e0ef1896a9006d4d61aab74f7281b4d3916737 Mon Sep 17 00:00:00 2001 From: "R. Garcia-Dias" Date: Thu, 3 Sep 2026 15:45:51 +0100 Subject: [PATCH 6/9] test(nnunet): assert warning fires before load_pickle Strengthen the postprocessing pickle test to record event ordering and assert the trust-boundary warning is emitted before the untrusted file is unpickled. Signed-off-by: R. Garcia-Dias --- .../apps/nnunet/test_nnunetv2_runner_command.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/apps/nnunet/test_nnunetv2_runner_command.py b/tests/apps/nnunet/test_nnunetv2_runner_command.py index 079005ade9..8c92d6946a 100644 --- a/tests/apps/nnunet/test_nnunetv2_runner_command.py +++ b/tests/apps/nnunet/test_nnunetv2_runner_command.py @@ -175,19 +175,27 @@ def test_postprocessing_pickle_warns_on_untrusted_file(self): "nnunetv2.utilities.file_path_utilities": fp_mod, } - load_pickle = mock.MagicMock(return_value=([], {})) + events = [] + + def _load_pickle(path): + events.append("load_pickle") + return [], {} + + def _warn(*args, **kwargs): + events.append("warn") + + load_pickle = mock.MagicMock(side_effect=_load_pickle) with mock.patch.dict(sys.modules, fake_modules): with mock.patch.object(ConfigParser, "load_config_file", return_value=runner.best_configuration): with mock.patch.object(nnunetv2_runner, "join", os.path.join): with mock.patch.object(nnunetv2_runner, "load_pickle", load_pickle): - with warnings.catch_warnings(record=True) as caught: - warnings.simplefilter("always") + with mock.patch.object(nnunetv2_runner.warnings, "warn", side_effect=_warn): runner.predict_ensemble_postprocessing( run_predict=False, run_ensemble=False, run_postprocessing=True ) load_pickle.assert_called_once_with("/tmp/attacker_controlled_postprocessing.pkl") - self.assertTrue(any("unpickling postprocessing_file" in str(item.message) for item in caught)) + self.assertEqual(events, ["warn", "load_pickle"]) if __name__ == "__main__": From 5c65534e20bdcc662ad6625e2f12ba810cd3cd51 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:46:47 +0000 Subject: [PATCH 7/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/apps/nnunet/test_nnunetv2_runner_command.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/apps/nnunet/test_nnunetv2_runner_command.py b/tests/apps/nnunet/test_nnunetv2_runner_command.py index 8c92d6946a..1553374748 100644 --- a/tests/apps/nnunet/test_nnunetv2_runner_command.py +++ b/tests/apps/nnunet/test_nnunetv2_runner_command.py @@ -16,7 +16,6 @@ import threading import types import unittest -import warnings from unittest import mock from monai.apps.nnunet import nnunetv2_runner From 23a4c8679979cd629c8f3cf350940f62dc9b5219 Mon Sep 17 00:00:00 2001 From: "R. Garcia-Dias" Date: Thu, 3 Sep 2026 18:07:46 +0100 Subject: [PATCH 8/9] DCO Remediation Commit for R. Garcia-Dias I, R. Garcia-Dias , hereby add my Signed-off-by to this commit: 224b4073af38bff5954e6c499f7d71127949a0ed Signed-off-by: R. Garcia-Dias From 0bab55e1a5b8a32ba038c3255124e300698c1377 Mon Sep 17 00:00:00 2001 From: "R. Garcia-Dias" Date: Thu, 3 Sep 2026 18:11:56 +0100 Subject: [PATCH 9/9] fix: address PR #9086 review feedback - tests/apps/nnunet/test_nnunetv2_runner_command.py: add docstrings to _load_pickle and _warn test helpers Signed-off-by: R. Garcia-Dias --- tests/apps/nnunet/test_nnunetv2_runner_command.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/apps/nnunet/test_nnunetv2_runner_command.py b/tests/apps/nnunet/test_nnunetv2_runner_command.py index 1553374748..7dc3bae60c 100644 --- a/tests/apps/nnunet/test_nnunetv2_runner_command.py +++ b/tests/apps/nnunet/test_nnunetv2_runner_command.py @@ -177,10 +177,24 @@ def test_postprocessing_pickle_warns_on_untrusted_file(self): events = [] def _load_pickle(path): + """Record a ``load_pickle`` call and return an empty postprocessing pipeline. + + Args: + path: path to the pickle file (unused). + + Returns: + A tuple of ``(postprocessing_fns, postprocessing_kwargs)``. + """ events.append("load_pickle") return [], {} def _warn(*args, **kwargs): + """Record a ``warnings.warn`` call. + + Args: + *args: positional arguments passed to ``warnings.warn``. + **kwargs: keyword arguments passed to ``warnings.warn``. + """ events.append("warn") load_pickle = mock.MagicMock(side_effect=_load_pickle)