Bottles icon indicating copy to clipboard operation
Bottles copied to clipboard

Escape braces for runner path

Open RushingAlien opened this issue 1 year ago • 1 comments

Description

Hello, as you all know braces () are commonly used in filenames, steam's Proton 9 is still named Proton 9.0 (Beta). The braces caused an sh syntax error as per attached issue. This PR escapes the braces for runner path.

Fixes #3407

Type of change

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

  • Use a runner with braces in the name
  • run a windows program
  • run legacy wine tools
    • winecfg
    • explorer
    • uninstaller
    • debugger
    • control panel
    • taskmgr

RushingAlien avatar Jun 29 '24 11:06 RushingAlien

Pylint result on modfied files:
************* Module bottles.backend.wine.winecommand
bottles/backend/wine/winecommand.py:90:4: W0102: Dangerous default value {} as argument (dangerous-default-value)
bottles/backend/wine/winecommand.py:90:4: R0913: Too many arguments (11/5) (too-many-arguments)
bottles/backend/wine/winecommand.py:135:12: W0105: String statement has no effect (pointless-string-statement)
bottles/backend/wine/winecommand.py:141:12: W0105: String statement has no effect (pointless-string-statement)
bottles/backend/wine/winecommand.py:397:20: W0105: String statement has no effect (pointless-string-statement)
bottles/backend/wine/winecommand.py:401:51: E1136: Value 'gpu['prime']['integrated']' is unsubscriptable (unsubscriptable-object)
bottles/backend/wine/winecommand.py:403:20: W0105: String statement has no effect (pointless-string-statement)
bottles/backend/wine/winecommand.py:182:8: W0612: Unused variable 'is_nvidia' (unused-variable)
bottles/backend/wine/winecommand.py:449:12: W0105: String statement has no effect (pointless-string-statement)
bottles/backend/wine/winecommand.py:458:12: W0105: String statement has no effect (pointless-string-statement)
bottles/backend/wine/winecommand.py:476:4: R0913: Too many arguments (6/5) (too-many-arguments)
bottles/backend/wine/winecommand.py:511:32: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
bottles/backend/wine/winecommand.py:520:21: W1514: Using open without explicitly specifying an encoding (unspecified-encoding)
bottles/backend/wine/winecommand.py:529:21: W1514: Using open without explicitly specifying an encoding (unspecified-encoding)
bottles/backend/wine/winecommand.py:545:20: W0105: String statement has no effect (pointless-string-statement)
bottles/backend/wine/winecommand.py:550:20: W0105: String statement has no effect (pointless-string-statement)
bottles/backend/wine/winecommand.py:555:20: W0105: String statement has no effect (pointless-string-statement)
bottles/backend/wine/winecommand.py:648:8: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
bottles/backend/wine/winecommand.py:659:8: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
bottles/backend/wine/winecommand.py:697:12: R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it (no-else-return)
bottles/backend/wine/winecommand.py:718:23: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)

github-actions[bot] avatar Jun 29 '24 11:06 github-actions[bot]

Why is manual escaping even needed here? 'm not familiar with the codebase, but the subprocess modules (which should be used) of Python does escaping automatic.

JakobDev avatar Jul 02 '24 12:07 JakobDev

See linked issue, and I do not know if the subprocess module is used or not, only that the sh command that bottles executes to start a program in wine fails in a syntax error if there's braces in path.

and the change I've made are additions to an existing manual escape for spaces. I'm just adding one for braces.

RushingAlien avatar Jul 02 '24 12:07 RushingAlien

Why is manual escaping even needed here? 'm not familiar with the codebase, but the subprocess modules (which should be used) of Python does escaping automatic.

Quoting is used everywhere in the codebase because subprocess.Popen is being called with shell=True. From a quick look, I'm not sure if that shell=True is really necessary, but removing it would require rewriting a lot of stuff, so manual quoting it is for now. Manually quoting should be just as safe, as long as we never forget to do it, like what happened here.

jntesteves avatar Jul 02 '24 13:07 jntesteves

@RushingAlien if you're not confortable with doing the change I suggested and changing the commit message to reflect that, I'll open a new PR to resolve this issue.

jntesteves avatar Jul 02 '24 13:07 jntesteves