constructor icon indicating copy to clipboard operation
constructor copied to clipboard

Fixes uninstaller breaking CMD.exe by corrupting registry keys

Open guimondmm opened this issue 2 years ago • 3 comments

Fixes https://github.com/ContinuumIO/anaconda-issues/issues/12938 and addresses https://github.com/ContinuumIO/anaconda-issues/issues/12735.

On 23 Oct 2020, https://github.com/conda/conda/commit/b4f3210bab7819a9a08e3038276cbb2d2498421c changed the regex pattern used by the init_cmd_exe_registry function in conda/core/initialize.py to configure the conda hook for CMD.exe on Windows:

@@ -1430 +1435 @@ def init_cmd_exe_registry(target_path, conda_prefix, reverse=False):
-            r'(\"[^\"]*?conda[-_]hook\.bat\")',
+            r'(if exist \"[^\"]*?conda[-_]hook\.bat\" \"[^\"]*?conda[-_]hook\.bat\")',

This changed the format of the string written by the conda init command to either of the following Windows Registry variables:

  • HKEY_LOCAL_MACHINE\Software\Microsoft\Command Processor\AutoRun
  • HKEY_CURRENT_USER\Software\Microsoft\Command Processor\AutoRun

However, the regex pattern used by the rm_regkeys function in constructor/nsis/_nsis.py was not updated accordingly.

Because of that oversight, when a Windows user who has run conda init after https://github.com/conda/conda/commit/b4f3210bab7819a9a08e3038276cbb2d2498421c attempts to uninstall Anaconda using Uninstall-Anaconda3.exe, the conda hook isn't deleted properly from the registry by the constructor/nsis/_nsis.py script, which breaks CMD.exe and causes system instability.

This is because the old regex pattern ((\s+&\s+)?\"[^\"]*?conda[-_]hook\.bat\") deleted by constructor/nsis/_nsis.py matches twice in the new pattern (if exist \"[^\"]*?conda[-_]hook\.bat\" \"[^\"]*?conda[-_]hook\.bat\") set by conda/core/initialize.py, and leaves behind if exist with trailing spaces.

When CMD.exe is launched without the /D flag, it attempts to read the AutoRun variable from the registry, and if it is set, it tries to execute the specified string first. The syntax of the if exist command is invalid, so CMD.exe silently terminates with exit code 1. This breaks the Command Prompt as well as batch files, and causes several apps to crash or misbehave. Windows repair tools like SFC.exe and DISM.exe don't detect the corrupted registry variables, and reinstalling Anaconda doesn't fix the issue. Unless the user knows about obscure CMD.exe flags and registry keys, their system is effectively hosed.

To address the issue, I've updated the regex pattern in the rm_regkeys function of constructor/nsis/_nsis.py to match both the older pattern and the newer one from the init_cmd_exe_registry function in conda/core/initialize.py.

guimondmm avatar Jun 22 '22 05:06 guimondmm

We require contributors to sign our Contributor License Agreement and we don't have one on file for @guimondmm.

In order for us to review and merge your code, please e-sign the Contributor License Agreement PDF. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature.

conda-bot avatar Jun 22 '22 05:06 conda-bot

Signed the CLA.

guimondmm avatar Jun 22 '22 17:06 guimondmm

@conda-bot check

jaimergp avatar Aug 15 '22 09:08 jaimergp

Thank you so much @guimondmm!

jaimergp avatar Sep 05 '22 08:09 jaimergp

Thank you so much! But, why not use the same method as conda init --reverse? That might be more robust to upstream pattern change. Moreover, it seems that current method will break AutoRun with value like <Conda AutoRun> & <Other App AutoRun>, by substituting it to & <Other App AutoRun>. But it is invalid to have beginning & in CMD.exe

YouJiacheng avatar Sep 14 '22 02:09 YouJiacheng

That's an excellent point. We can investigate how that works in a separate PR. The reason would be that constructor wasn't using a full conda until version 3, so the uninstall code probably predates the possibility of using conda init --reverse.

jaimergp avatar Sep 14 '22 08:09 jaimergp

This is still an issue with the installers available from https://docs.conda.io/en/latest/miniconda.html I'm not sure if it's relevant but when I look at the merge commit's associated workflow, the "Upload" action was skipped.

This is a critical issue as is pointed out in the fix, because it can leave the OS severely broken, and should make sure it's not only merged but also uploaded to the installers.

AngusMaiden avatar Nov 25 '22 04:11 AngusMaiden

The installers you can currently find in that website are built by a separate constructor fork. Pinging @pseudoyim for visibility / awareness.

jaimergp avatar Dec 05 '22 10:12 jaimergp