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 #40973 win_package fails to uninstall a package whose registry Un… #49823

Open
wants to merge 3 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@rufinio
Copy link

rufinio commented Dec 12, 2018

…installString is quoted

SUMMARY

If UninstallString is quoted and has no extra args in there, remove leading and trailing double-quotes.
Fixes #40973

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

win_package

ADDITIONAL INFORMATION

win_package fails to uninstall a package whose registry UninstallString is quoted.
#40973

@ansibot

This comment has been minimized.

@@ -319,7 +319,11 @@ if ($state -eq "absent") {
$cleanup_artifacts += $local_path
} elseif ($program_metadata.location_type -eq [LocationType]::Empty -and $program_metadata.msi -ne $true) {
# TODO validate the uninstall_string to see if there are extra args in there
$local_path = $program_metadata.uninstall_string
if ((@($program_metadata.uninstall_string).Count -eq 1) -and ($program_metadata.uninstall_string.StartsWith("`""))) {

This comment has been minimized.

@jborean93

jborean93 Dec 12, 2018

Contributor

What's the reasoning behind the @($program_metadata.uninstall_string).Count -eq 1 part?. I would also change the StartWith section to be '"' to not worry about escaping the double quotes. Same with the the Trim methods.

@ansibot ansibot removed the needs_triage label Dec 13, 2018

@ansibot ansibot removed the small_patch label Dec 13, 2018

@@ -319,7 +319,12 @@ if ($state -eq "absent") {
$cleanup_artifacts += $local_path
} elseif ($program_metadata.location_type -eq [LocationType]::Empty -and $program_metadata.msi -ne $true) {
# TODO validate the uninstall_string to see if there are extra args in there
$local_path = $program_metadata.uninstall_string
$parsed_uninstall_string = @(ParseCommandLine $program_metadata.uninstall_string)

This comment has been minimized.

@jborean93

jborean93 Dec 17, 2018

Contributor

ParseCommandLine isn't a cmdlet I'm aware off which means the code isn't actually doing anything. Can't you just do

$local_path = $program_metadata.uninstall_string
if ($local_path.StartsWith('"') -and $local_path.EndsWith('"')) {
    $local_path = $local_path.Substring(1, $local_path.Length - 2)
}

This comment has been minimized.

@rufinio

rufinio Dec 18, 2018

Author

ParseCommandLine was in Ansible.ModuleUtils.CommandUtil.psm1 and thrown away with 190d1ed

public static string[] ParseCommandLine(string lpCommandLine)
{
int numArgs;
IntPtr ret = CommandLineToArgvW(lpCommandLine, out numArgs);
if (ret == IntPtr.Zero)
throw new Win32Exception("Error parsing command line");
IntPtr[] strptrs = new IntPtr[numArgs];
Marshal.Copy(ret, strptrs, 0, numArgs);
string[] cmdlineParts = strptrs.Select(s => Marshal.PtrToStringUni(s)).ToArray();
Marshal.FreeHGlobal(ret);
return cmdlineParts;
}

What about values for uninstall_string like
"C:\Program Files\myprog\uninst.exe" Folder="C:\Documents and Settings\All Users\Application Data\myprog"

This comment has been minimized.

@jborean93

jborean93 Dec 18, 2018

Contributor

Ahh ok I see what you were trying to do. Unfortunately that wouldn't have worked as you are calling the .NET method but you haven't specified the class it was in. You would have technically had to do [Ansible.CommandUtil]::ParseCommandLine($program_metadata.uninstall_string) but the namespace has been changed from that commit you mentioned so it won't work anymore.

The code was changed to better align with the newer C# module util structure we have recently introduced. At the time of the commit I couldn't find it being used anywhere else and we decided to make the change.

Ultimately I think what we should be doing is to not process the uninstall registry value at all and just pass that directly into Run-Command. That way any comments or arguments that are stored by the app are preserved and we can just append our arguments onto the end of the string if they have been set by the user. This would mean some changes further down to not call Argv-ToString but it should still be possible in this case.

@ansibot ansibot added the stale_ci label Dec 26, 2018

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 20, 2019

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.