ProgerXP / Notepad2e

Word highlighting, simultaneous editing, split views, math evaluation, un/grep, comment reformatting, UAC elevation, complete regexps (PCRE), Lua lexers, DPI awareness and more (XP+)
Other
373 stars 52 forks source link

Don't interpret %envvars% in pathname when opening file #193

Open tumagonx opened 5 years ago

tumagonx commented 5 years ago

Another weird behavior of notepad2 for a long time

Case : create the following file and path in explorer D:\data\%appdata%\foo\bar.txt try open bar.txt using notepad2 e.g. via imageexecution hack or direct drag and drop onto notepad2e.exe it return error unable to find "D:\data\C:\Users\root\Application Data\bar.txt"

notepad or wordpad or any "normal" programs don't do that. note: the usecase is by Thinapp/Thinstall which create such directory structure, but is still a valid case of how to open file properly.

ProgerXP commented 5 years ago

I can confirm this: attempt to open a file with valid %env% vars syntax fails due to those vars' expansion.

Apparently the original intention of Notepad2 is to not rely on Windows shell expanding those vars. I guess we'll need to disable this by default and add a command-line flag like /expand which will perform substitutions on the command-line arguments.

cshnik commented 5 years ago

Added command line option /expandenv (default value is 0). Provide /expandenv=1 to enable legacy behavior.

ProgerXP commented 5 years ago

It doesn't work for me.

However, the fix is incorrect anyway because it's made inside ExpandEnvironmentStringsEx which is used in many places, e.g. for locating INI file. Making /expandenv a global option affecting all these places is wrong - it should only affect paths read from the command line, not elsewhere.

The correct place for this correction would be around line 6330 but I don't see ExpandEnvironmentStringsEx (PathFixBackslashes call) there so it must be somewhere later in the initialization logic. Still, it should only be applied to command line pathnames.

cshnik commented 5 years ago

However, the fix is incorrect anyway because it's made inside ExpandEnvironmentStringsEx which is used in many places, e.g. for locating INI file. Making /expandenv a global option affecting all these places is wrong - it should only affect paths read from the command line, not elsewhere.

Here are some points:

  1. Global option looks smooth and easier for understanding.
  2. I agree that providing default value to alter legacy behavior and making this option global is risky.
  3. Correct local fix for startup open file command is impossible due to the next fact: MRU_AddFile()-method uses PathRelativeToApp which heavily uses transform path logic. This affects File> Recent (History)-feature (when RelativeFileMRU is set to 1) and caused preserving of incorrect path. This also breaks Recall Previous feature. I assume there can be some other features which would be unusable without additional fixes.
  4. Implemented global option affects only generic methods PathRelativeToApp and ExpandEnvironmentStringsEx which are called in multiple places. These changes were tested in different scenarios.
  5. Regarding the INI-files, potentially affected case is when INI-path with environmental variables is provided from the command line, e.g. Notepad2e.exe /f %APPDATA%. FindIniFile() (utilizes ExpandEnvironmentStringsEx()) is called once from WinMain. Affected code depends on CheckIniFile()-result which internally uses standard ExpandEnvironmentStrings function to expand environmental variables when checking provided path. I've made different tests using /f switch to provide INI path and different /expandenv modes and found no difference: in all cases loading of INI file works correctly.

It doesn't work for me.

Here are the test cases to check the fix:

  1. Create the following file and path in explorer: C:\data%appdata%\1.txt
  2. Run cmd.exe, enter Notepad2e startup command with quoted % character and note result, e.g:

    notepad2e.exe C:\data^%appdata^%\1.txt

  3. Drag and drop 1.txt file into running instance of Notepad2e, note the result.
  4. Replace Windows notepad using imageexecution and double click on 1.txt, note the result.

Still, it should only be applied to command line pathnames.

Please confirm if you insist on the rollback.

tumagonx commented 5 years ago

Looks like indeed a quirk one, more complicated than I thought which make me wonder whats the original real world use-cases of that quirk?