Skip to content

Avoid shell=True when invoking PACKMOL - #1347

Merged
chrisjonesBSU merged 3 commits into
mosdef-hub:mainfrom
rayair250-droid:fix/packmol-shell-injection
Jul 24, 2026
Merged

Avoid shell=True when invoking PACKMOL#1347
chrisjonesBSU merged 3 commits into
mosdef-hub:mainfrom
rayair250-droid:fix/packmol-shell-injection

Conversation

@rayair250-droid

Copy link
Copy Markdown
Contributor

Summary

PACKMOL is currently launched through a shell with an interpolated temp path:

proc = Popen(
    f"{PACKMOL} < {packmol_inp.name}",
    stdin=PIPE, stdout=PIPE, stderr=PIPE,
    universal_newlines=True, shell=True,
)

Two problems:

  1. Shell-injection anti-pattern — building a shell command line by interpolating a path into an f-string with shell=True.
  2. Breaks on spaces / shell metacharacters — if the temp file path contains a space (e.g. TMPDIR under a user directory with spaces, common on macOS/Windows), the < redirection targets the wrong token and PACKMOL fails.

The shell was only being used for the < input redirection; stdin=PIPE was set but nothing was ever written to it.

Fix

Feed the input file directly as stdin and pass the executable as an argument list, dropping the shell. PACKMOL is a resolved absolute path from shutil.which("packmol"), so no shell word-splitting is needed.

with open(packmol_inp.name) as inp_file:
    proc = Popen(
        [PACKMOL],
        stdin=inp_file, stdout=PIPE, stderr=PIPE,
        universal_newlines=True,
    )
    out, err = proc.communicate()

Behavior is unchanged (PACKMOL reads its config from stdin exactly as before), and the call is now injection-safe and robust to paths with spaces.

Popen was called with shell=True and an f-string interpolating a temp-file
path (f"{PACKMOL} < {packmol_inp.name}"). This is a shell-injection
anti-pattern and breaks whenever the temp path contains spaces or shell
metacharacters (e.g. a TMPDIR with spaces).

Pass the executable as an argument list and feed the input file directly as
stdin instead of relying on a shell redirection. Behavior is unchanged (the
shell was only used for the '<' redirection), and PACKMOL is a resolved path
from shutil.which().
@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.07%. Comparing base (51e6c9b) to head (8dfff46).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1347   +/-   ##
=======================================
  Coverage   86.07%   86.07%           
=======================================
  Files          52       52           
  Lines        4897     4898    +1     
=======================================
+ Hits         4215     4216    +1     
  Misses        682      682           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@chrisjonesBSU
chrisjonesBSU merged commit bcca2c1 into mosdef-hub:main Jul 24, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants