Skip to content

FIX: Handle non-UTF-8 output gracefully in run_subprocess - #14425

Open
ishaaaan17 wants to merge 3 commits into
mne-tools:mainfrom
ishaaaan17:fix/run-subprocess-decode-resilience
Open

ishaaaan17 wants to merge 3 commits into
mne-tools:mainfrom
ishaaaan17:fix/run-subprocess-decode-resilience

Conversation

@ishaaaan17

Copy link
Copy Markdown
Contributor

reference issue (if any)

none

what does this implement/fix?

in mne.utils.run_subprocess, the stdout and stderr are decoded using decode("utf-8"). when some external command gives non-utf-8 bytes, it raises UnicodeDecodeError and the function crashes.

in this pr i have added errors="replace" to both out.decode("utf-8") and err.decode("utf-8"). i have also added test_run_subprocess_non_utf8 to check that the non-utf-8 bytes on stdout and stderr are handled properly without raising any error.

additional information

i wrote the fix myself, and gemini helped me to draft the initial test. after that i reviewed it, formatted it and tested everything locally.

When external commands or subprocesses emit non-UTF-8 binary output on stdout or stderr, decoding with default strict utf-8 raised UnicodeDecodeError and crashed run_subprocess. Use errors='replace' to decode gracefully without raising unhandled exceptions.
@scott-huberty

Copy link
Copy Markdown
Contributor

@ishaaaan17 I'm curious if there was a practical motivation for this fix? Like did you hit this error when using MNE-Python?

@ishaaaan17

Copy link
Copy Markdown
Contributor Author

hi @scott-huberty,

to be honest with you, i did not face this in any normal MNE analysis pipeline. i noticed it while going through the error handling paths in run_subprocess.

the main reason was the external binaries on different platforms (like FreeSurfer or OpenMEEG tools called through bem.py / surface.py / forward.py). on Windows or localized systems, these subprocesses can sometimes give stdout/stderr in the system code page (for example Windows-1252, or non-utf-8 characters in localized error messages or paths). when strict utf-8 decoding fails with UnicodeDecodeError, it stops before the actual process output or exit code can be logged, so we cannot see what the tool actually did.

adding errors="replace" looked like a simple and safe way to make sure run_subprocess always returns the output without crashing on decoding, and it has no extra cost.

but if you feel it is not needed or too defensive in practice, i understand, and i am happy to close it.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants