Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues with win_service #49327

Merged
merged 10 commits into from Sep 5, 2018

Conversation

Projects
None yet
5 participants
@twangboy
Copy link
Contributor

commented Aug 25, 2018

What does this PR do?

Add shlex.quote
Properly quote the path to the binary (bin_path) on create and modify. It was removing double quotes. Double quotes are needed for paths to binaries with spaces. Single quotes will fail also.
Don't crash on service.stop when the service is already stopped
Don't crash on service.delete when the service is not present
Fix issues with the exception handling

What issues does this PR fix or reference?

#48828
#49234

Tests written?

No

Commits signed with GPG?

Yes

@salt-jenkins salt-jenkins requested a review from saltstack/team-windows Aug 25, 2018

@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2018

I still need to write some tests for this

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Aug 25, 2018

ping @ciiqr

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Aug 25, 2018

  • Consider using from salt.utils.win_functions import escape_argument or escape_for_cmd_exe instead of shlex.quote
  • We should not convert single quotes as its a valid file name, even if its a bad idea for someone to use it in a filename.
  • Suggest you check for reporting changes impacts to states/service.py with the start/stop returning true when already started/stopped etc.

The other alternative is to check for begins with " then leave it alone, otherwise wrap " around it after running escape_argument or escape_for_cmd_exe

@twangboy twangboy force-pushed the twangboy:fix_win_service branch from f242b9e to 62567e2 Aug 27, 2018

@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

@damon-atkins
Neither of the win_functions commands will work in this case as they actually escape the quotes at the beginning and end. For example, C:\Temp Dir\test.exe becomes ^"C:\\Temp Dir\test.exe^". The command needs to be passed with unescaped double quotes.

Since this is the service module, I can't think of an instance where the path to an exe would start or end with a single quote. It would always end in .exe and would always start with the drive letter.

The _cmd_quote function only strips quotes from the beginning and the end. It doesn't strip single quotes from anything inside. This produces a valid value no matter how the bin_path is quoted in the state file.

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

@twangboy I was wrong about escape_for_cmd_exe() as it should only be used for cmd.exe

The line bin_path = bin_path.strip('"') is the real bug, as it did not perform any checking and it removed all " anywhere in the string.

Two fixes available
A) remove bin_path = bin_path.strip('"') from the original version and improve the documentation (if needed) and examples (if needed), First Choice
OR
B)

def _cmd_quote(execute_path):  # add to salt.utils.win_functions
  if execute_path.startswith(") and execute_path.endswith(",1):
      return execute_path
  return '"{0}"'.format(execute_path)

The above will support

bin_path: hello world.exe
bin_path: "hello world.exe"   # here yaml removes the "
bin_path: '"hello world.exe"  # here yaml removes the '

And not support

bin_path: "'hello world.exe'" # which a windows person should not do.

And if this code is introduced here to help, then it needs to be introduced in other places for consistency, is that doable or will it require to much if windows

Example of inconsistency is the include supported in states but not supported in pillars, which catches me out.


Stripping off ' is doing to much for the user. And the user would need to do some yaml with cmd: "'prg.exe'" I would have thought most Windows admin would known to quote with " vs a Unix admin trying to work out windows.

Please go with option A.

twangboy added some commits Aug 25, 2018

Fix issues with win_service
Add shlex.quote
Properly quote the path to the binary (bin_path) on create and modify
Don't crash on service.stop when the service is already stopped
Don't crash on service.delete when the service is not present
Fix issues with the exception handling

@twangboy twangboy force-pushed the twangboy:fix_win_service branch from 97de49b to 97567af Aug 28, 2018

@dwoz dwoz self-requested a review Aug 28, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2018

@dwoz can you take a look here?

@dwoz

dwoz approved these changes Aug 30, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2018

@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2018

@rallytime This may be a flaky test. I can't replicate it manually. Is there a way to re run?

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Sep 3, 2018

Might be related and might not, since this PR also whitelists that test for the first time. Still, though, we'll want to mark it as a flaky or fix the test before we merge this. I'll re-reun tests now to see what comes back.

Mike Place and others added some commits Sep 3, 2018

Mike Place

@rallytime rallytime merged commit 34e5174 into saltstack:2017.7 Sep 5, 2018

7 of 10 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has failed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has failed
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details

@twangboy twangboy deleted the twangboy:fix_win_service branch Sep 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.