notepad-plus-plus / wingup

WinGup - Generic Updater for Windows
http://wingup.org/
GNU Lesser General Public License v3.0
47 stars 31 forks source link

Fix installer download when temp. path is not set #18

Closed mere-human closed 2 years ago

mere-human commented 2 years ago

Use a fallback TMP or TEMP and then use the current directory.

Fix #16

mere-human commented 2 years ago

How to check it without harming the local machine environment? Just unset any of the variables in debugger. Project > Properties: image

mere-human commented 2 years ago

@donho I think this is pretty important to fix because:

  1. Absence of TEMP leads to crash which is not a good user experience.
  2. Using _wgetenv may lead to a security problem.

https://stackoverflow.com/questions/48568707/getenv-function-may-be-unsafe-really

donho commented 2 years ago

@mere-human

How to check it without harming the local machine environment? Just unset any of the variables in debugger. Project > Properties:

It's good. But I need a real crash to validate this PR. Removing both TMP & TEMP doesn't make GUP crash, and GUP updates Notepad++ with no problem.

mere-human commented 2 years ago

But I need a real crash to validate this PR.

@donho ok, try this:

mere-human commented 2 years ago

Or this: https://stackoverflow.com/questions/13222724/command-line-to-remove-an-environment-variable-from-the-os-level-configuration

But be careful!

mere-human commented 2 years ago

@donho Do you consider merging this before RC?

jurekl commented 2 years ago

@donho

It's good. But I need a real crash to validate this PR. Removing both TMP & TEMP doesn't make GUP crash, and GUP updates Notepad++ with no problem.

I am providing a link to my specification of the error description https://github.com/notepad-plus-plus/wingup/issues/16#issuecomment-913205305

donho commented 2 years ago

@mere-human I'm doing RC2, and I'm wondering if you wanna add %UserProfile%\Downloads in this PR. If yes, could it be done asap? Otherwise we can always update GUP in the next next release, or RC3 (if any).

mere-human commented 2 years ago

I'm doing RC2, and I'm wondering if you wanna add %UserProfile%\Downloads in this PR. If yes, could it be done asap?

@donho I can do it later today (in ~4 hours) since I need to do some testing. If it's too late, then proceed with submitting this PR and I can add the Downloads folder handling in a separate PR.

donho commented 2 years ago

If it's too late, then proceed with submitting this PR and I can add the Downloads folder handling in a separate PR.

Please do it in the same PR. RC2 is already available, so let's integrate the new GUP in the 8.1.6 release.

mere-human commented 2 years ago

Verified on Windows 10 and on Windows 7. Here's a result of running an installer from terminal without TMP, TEMP and AppData: image

donho commented 2 years ago

ARM64 build error:

image

mere-human commented 2 years ago

ARM64 build error:

Sorry, I forgot about ARM64 build. That's why WinGUP needs this https://github.com/notepad-plus-plus/wingup/issues/17 I'll fix it.

mere-human commented 2 years ago

Shall we remove ARM mentions from the .vcxproj file? It seems it was added by mistake https://github.com/notepad-plus-plus/wingup/blob/master/vcproj/GUP.vcxproj#L57 image

donho commented 2 years ago

@mere-human

The request is quite clear:

Use Win32 API instead of COM, or remove the following part completely.

    LPWSTR path = nullptr;
    const HRESULT hr = ::SHGetKnownFolderPath(FOLDERID_Downloads, 0, nullptr, &path);
    if (SUCCEEDED(hr))
    {
        std::wstring result = path;
        CoTaskMemFree(path);
        return result;
    }

I don't want COM in WinGup project.

mere-human commented 2 years ago

I don't want COM in WinGup project.

It's already in x64 build. Ok, I can remove this part. Or compile it for x64 but not for ARM64.

donho commented 2 years ago

Shall we remove ARM mentions from the .vcxproj file? It seems it was added by mistake https://github.com/notepad-plus-plus/wingup/blob/master/vcproj/GUP.vcxproj#L57

Leave it please. I will take care of it myself if needed.

donho commented 2 years ago

Thank you @mere-human