WinUpdater: Quote path for args when relaunching Dolphin.

[AdmiralCurtiss]

This should fix https://forums.dolphin-emu.org/Thread-recent-update-triggering-this-error-on-auto-update

I think just adding quotes on both ends is enough, because a valid path on Windows cannot contain quotes by itself.

[shuffle2]

since it only broke recently, i think it's better to only do this for CreateProcess path.

https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa

also, if it's taking the ShellExecute path...it means the user is running dolphin as admin (or, the updater decided it needed to elevate), which Is Bad. And it appears to be in My Documents, which doesn't require admin, anyway.

[AdmiralCurtiss]

Fair enough.

[shuffle2]

Also, you can use test-updater.py to repro the bug and check that it’s fixed (passing dir param to temporarydirectory). Maybe the test should just be changed to ensure there’s always a space and Unicode in the path or something…

[shuffle2]

According to msdn both args to create process need quotes. Try using the test script :)

[AdmiralCurtiss]

I did test this yesterday and it works as written, but yeah maybe some dumb Windows fallback is saving me here...

(seriously though why is the API like this? yeah backcompat and all but this really feels like someone should have made a saner variant in a later windows version...)

[AdmiralCurtiss]

Btw the way I'm reading the MSDN doc is that the application name only needs to be quoted if either lpApplicationName or lpCommandLine is NULL (since that causes the non-NULL of them to be interpreted as the command line); and if neither is NULL then the lpApplicationName is the path to the application (which doesn't need quotes, based on the security remark at the bottom) and the lpCommandLine does need quotes since it doesn't know where to split the command line arguments otherwise.

[AdmiralCurtiss]

Okay, I tested this out under the following scenario taking into account the Security stuff on the MSDN page:

---

c:\Test\a dir\a.exe is the 'correct' executable that we want to launch. It's a simple C# WinForms application that prints 'Correct Executable', and in the textbox its own path, then its working directory, then its arguments.

C# Code

namespace WinFormsApp1 {
	internal static class Program {
		/// <summary>
		///  The main entry point for the application.
		/// </summary>
		[STAThread]
		static void Main(string[] args) {
			// To customize application configuration such as set high DPI settings or default font,
			// see https://aka.ms/applicationconfiguration.
			ApplicationConfiguration.Initialize();
			Application.Run(new Form1(args));
		}
	}

	public partial class Form1 : Form {
		public Form1(string[] args = null) {
			InitializeComponent();

			textBox1.Text += System.Reflection.Assembly.GetExecutingAssembly().Location;
			textBox1.Text += "\r\n";
			textBox1.Text += Directory.GetCurrentDirectory();
			textBox1.Text += "\r\n";
			textBox1.Text += "\r\n";
			foreach (string s in args) {
				textBox1.Text += s + "\r\n";
			}
		}
	}
}

c:\Test\a.exe is largely identical but prints 'Wrong Executable' instead.

---

We now attempt to launch this in a few different ways using CreateProcessW():

Both parameters, both unquoted

#include <string>

#include <Windows.h>

int main(int argc, char** argv) {
	std::wstring unquoted = L"c:\\Test\\a dir\\a.exe";
	std::wstring quoted = L"\"" + unquoted + L"\"";

	std::wstring lpApplicationName = unquoted;
	std::wstring lpCommandLine = unquoted;

	STARTUPINFOW startup_info{ .cb = sizeof(startup_info) };
	PROCESS_INFORMATION process_info;
	BOOL rv = CreateProcessW(lpApplicationName.data(), lpCommandLine.data(), nullptr, nullptr, TRUE, 0, nullptr, nullptr, &startup_info, &process_info);
	printf("%s\n", rv ? "launch success" : "launch failed");
}

==> Launches correct executable (prints launch success) with:

c:\Test\a dir\a.dll
c:\Test

dir\a.exe

So we launched the correct file, but with the argument dir\a.exe. If we delete c:\Test\a dir\a.exe nothing gets launched (prints launch failed).

Both parameters, both quoted

#include <string>

#include <Windows.h>

int main(int argc, char** argv) {
	std::wstring unquoted = L"c:\\Test\\a dir\\a.exe";
	std::wstring quoted = L"\"" + unquoted + L"\"";

	std::wstring lpApplicationName = quoted;
	std::wstring lpCommandLine = quoted;

	STARTUPINFOW startup_info{ .cb = sizeof(startup_info) };
	PROCESS_INFORMATION process_info;
	BOOL rv = CreateProcessW(lpApplicationName.data(), lpCommandLine.data(), nullptr, nullptr, TRUE, 0, nullptr, nullptr, &startup_info, &process_info);
	printf("%s\n", rv ? "launch success" : "launch failed");
}

==> Does not launch anything. Prints launch failed.

Both parameters, lpApplicationName unquoted, lpCommandLine quoted

#include <string>

#include <Windows.h>

int main(int argc, char** argv) {
	std::wstring unquoted = L"c:\\Test\\a dir\\a.exe";
	std::wstring quoted = L"\"" + unquoted + L"\"";

	std::wstring lpApplicationName = unquoted;
	std::wstring lpCommandLine = quoted;

	STARTUPINFOW startup_info{ .cb = sizeof(startup_info) };
	PROCESS_INFORMATION process_info;
	BOOL rv = CreateProcessW(lpApplicationName.data(), lpCommandLine.data(), nullptr, nullptr, TRUE, 0, nullptr, nullptr, &startup_info, &process_info);
	printf("%s\n", rv ? "launch success" : "launch failed");
}

==> Launches correct executable (prints launch success) with:

c:\Test\a dir\a.dll
c:\Test

So the process launched and did not get any arguments, exactly what we want.

Both parameters, lpApplicationName quoted, lpCommandLine unquoted

#include <string>

#include <Windows.h>

int main(int argc, char** argv) {
	std::wstring unquoted = L"c:\\Test\\a dir\\a.exe";
	std::wstring quoted = L"\"" + unquoted + L"\"";

	std::wstring lpApplicationName = quoted;
	std::wstring lpCommandLine = unquoted;

	STARTUPINFOW startup_info{ .cb = sizeof(startup_info) };
	PROCESS_INFORMATION process_info;
	BOOL rv = CreateProcessW(lpApplicationName.data(), lpCommandLine.data(), nullptr, nullptr, TRUE, 0, nullptr, nullptr, &startup_info, &process_info);
	printf("%s\n", rv ? "launch success" : "launch failed");
}

==> Does not launch anything. Prints launch failed.

For completeness sake:

lpCommandLine only, unquoted

#include <string>

#include <Windows.h>

int main(int argc, char** argv) {
	std::wstring unquoted = L"c:\\Test\\a dir\\a.exe";
	std::wstring quoted = L"\"" + unquoted + L"\"";

	//std::wstring lpApplicationName;
	std::wstring lpCommandLine = unquoted;

	STARTUPINFOW startup_info{ .cb = sizeof(startup_info) };
	PROCESS_INFORMATION process_info;
	BOOL rv = CreateProcessW(NULL, lpCommandLine.data(), nullptr, nullptr, TRUE, 0, nullptr, nullptr, &startup_info, &process_info);
	printf("%s\n", rv ? "launch success" : "launch failed");
}

==> Launches the wrong executable (launch success) with:

c:\Test\a.dll
c:\Test

dir\a.exe

That's not too surprising, that's exactly what the MSDN article warns against doing.

lpApplicationName only, unquoted

#include <string>

#include <Windows.h>

int main(int argc, char** argv) {
	std::wstring unquoted = L"c:\\Test\\a dir\\a.exe";
	std::wstring quoted = L"\"" + unquoted + L"\"";

	std::wstring lpApplicationName = unquoted;
	//std::wstring lpCommandLine = unquoted;

	STARTUPINFOW startup_info{ .cb = sizeof(startup_info) };
	PROCESS_INFORMATION process_info;
	BOOL rv = CreateProcessW(lpApplicationName.data(), NULL, nullptr, nullptr, TRUE, 0, nullptr, nullptr, &startup_info, &process_info);
	printf("%s\n", rv ? "launch success" : "launch failed");
}

==> Launches the correct executable (launch success) with:

c:\Test\a dir\a.dll
c:\Test

This one actually surprises me! I would have expected this to behave the same as the lpCommandLine only variant based on the documentation.

If we delete the correct executable this fails. (launch failed)

lpApplicationName only, quoted

#include <string>

#include <Windows.h>

int main(int argc, char** argv) {
	std::wstring unquoted = L"c:\\Test\\a dir\\a.exe";
	std::wstring quoted = L"\"" + unquoted + L"\"";

	std::wstring lpApplicationName = quoted;
	//std::wstring lpCommandLine = unquoted;

	STARTUPINFOW startup_info{ .cb = sizeof(startup_info) };
	PROCESS_INFORMATION process_info;
	BOOL rv = CreateProcessW(lpApplicationName.data(), NULL, nullptr, nullptr, TRUE, 0, nullptr, nullptr, &startup_info, &process_info);
	printf("%s\n", rv ? "launch success" : "launch failed");
}

==> Fails to launch anything. (launch failed)

lpCommandLine only, quoted

#include <string>

#include <Windows.h>

int main(int argc, char** argv) {
	std::wstring unquoted = L"c:\\Test\\a dir\\a.exe";
	std::wstring quoted = L"\"" + unquoted + L"\"";

	//std::wstring lpApplicationName = quoted;
	std::wstring lpCommandLine = quoted;

	STARTUPINFOW startup_info{ .cb = sizeof(startup_info) };
	PROCESS_INFORMATION process_info;
	BOOL rv = CreateProcessW(NULL, lpCommandLine.data(), nullptr, nullptr, TRUE, 0, nullptr, nullptr, &startup_info, &process_info);
	printf("%s\n", rv ? "launch success" : "launch failed");
}

==> Launches the correct executable (launch success) with:

c:\Test\a dir\a.dll
c:\Test

If we delete the executable it fails. (launch failed)

[AdmiralCurtiss]

Based on this:

• Adding quotes to the first parameter is definitely wrong, it never launches anything if you do that.
• Adding quotes to the second parameter is correct.
• Leaving the second parameter NULL seems to behave exactly like passing no arguments. The documentation seems to disagree with this (The lpCommandLine parameter can be NULL. In that case, the function uses the string pointed to by lpApplicationName as the command line.) so I'd avoid doing this.

[AdmiralCurtiss]

tldr: The PR should be correct as-is. I'll also test ShellExecuteW() shortly, just so we know what's up there...

[AdmiralCurtiss]

#include <string>

#include <Windows.h>

int main(int argc, char** argv) {
	std::wstring unquoted = L"c:\\Test\\a dir\\a.exe";
	std::wstring quoted = L"\"" + unquoted + L"\"";
	ShellExecuteW(nullptr, nullptr, L"explorer.exe", unquoted.c_str(), nullptr, SW_SHOW);;
}

Launches the correct executable, regardless of if I pass it quoted or unquoted. So that should also be fine as-is.

If I delete the executable it just opens Explorer (in My Documents), which is weird but at least not harmful.

[MayImilae]

It's been confirmed that this resolved the issue. https://forums.dolphin-emu.org/Thread-recent-update-triggering-this-error-on-auto-update?pid=529406#pid529406