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

Crash if trying to open existing file named "file." using notepad++ \\?\c:\file. #12453

Closed
white-gthb opened this issue Nov 2, 2022 · 28 comments
Assignees

Comments

@white-gthb
Copy link

white-gthb commented Nov 2, 2022

Description of the Issue

Crash if trying to open an existing file with file name ending with a dot using notepad++ \\?\c:\file.

Steps to Reproduce the Issue

  1. In elevated command prompt execute: echo 123 > \\?\c:\file.
  2. Use Windows Run (⊞ Win+R) to execute: notepad++ \\?\c:\file.

Expected Behavior

Some error if this is not supported, but no crash.

Actual Behavior

Notepad++ crashes.

Debug Information

Notepad++ v8.4.6 (64-bit)
Build time : Sep 25 2022 - 19:51:39
Path : C:\Program Files\Notepad++\notepad++.exe
Command Line :
Admin mode : OFF
Local Conf mode : OFF
Cloud Config : OFF
OS Name : Windows 11 (64-bit)
OS Version : 22H2
OS Build : 22621.755
Current ANSI codepage : 1252
Plugins :
DSpellCheck (1.4.23)
HexEditor (0.9.12)
mimeTools (2.8)
NppConverter (4.4)
NppExport (0.4)

@xomx
Copy link
Contributor

xomx commented Nov 3, 2022

The easiest solution - do not use such filenames, it is against the Windows naming conventions:

"Do not end a file or directory name with a space or a period. Although the underlying file system may support such names, the Windows shell and user interface does not..."

That is the problem with the "\\?\", it effectively disables the Windows OS filenaming checks and modifications:

"...For file I/O, the "\\?\" prefix to a path string tells the Windows APIs to disable all string parsing and to send the string that follows it straight to the file system... Many but not all file I/O APIs support "\\?\"; ..."

You can easily check yourself that other programs will have a problem too, e.g. try to open that file in Windows Notepad - simply continue in your cmd-window by entering this command: notepad.exe \\?\c:\file.
WinNotepad_cannot_find_file_with_dot_at_end

Note that the problematic dot at the end of the file name is missing from the above Notepad image! This is the source of this N++ crash - in the Notepad_plus::doOpen func the GetFullPathName WINAPI lies to N++ by cutting out the ending dot from the filename (this then leads to unexpected doOpen wild recursion in the following code and to the crash later).

If I find a spare time, I try a patch for this, because of the N++ should not crash in such situation but rather show an error/warning like the Windows Notepad.

Edit:
Crash/patch info for someone with more spare time than me:

  • the above info, about the GetFullPathName WINAPI lying to us, is true
  • but the crash wild recursion here is caused by N++ incorrect treating of the \\?\ filename-heading as a globbing

@white-gthb
Copy link
Author

Thanks for your professional response. I already knew the information about Window naming conventions, \\?\ and Windows Notepad, but it is certainly useful for other readers.

If I find a spare time, I try a patch for this, because of the N++ should not crash in such situation but rather show an error/warning like the Windows Notepad.

That's why I created the issue (see Expected Behavior).

@nickdavi
Copy link

Hello, I am a first time contributor. I built the program from source and believe I have fixed this bug. Right now I just have the message box displaying "{filename} cannot be opened." Is this acceptable? or is there a better way to handle this?

@alankilborn
Copy link
Contributor

alankilborn commented Nov 30, 2022

@nickdavi

or is there a better way to handle this?

Why not exactly as @xomx shows in the screenshot here?: #12453 (comment)


I am a first time contributor

Technically you aren't a contributor until something of yours gets accepted. :-)

@white-gthb
Copy link
Author

Why not exactly as @xomx shows in the screenshot here?: #12453 (comment)

Note that at the moment, when using no namespace prefixing (c:\newfile), a similar message is displayed when a file does not exist, but the error is simply ignored when using the \\?\ namespace prefix (\\?\c:\newfile).

@nickdavi
Copy link

nickdavi commented Nov 30, 2022

Hi @white-gthb, I attempted to trace back the issue and it appears that when \\?\ is entered into the command line parameters, the function "loadCommandLineParams" in Notepad_plus.cpp reads the commandLine argument (which gets turned into the fileName in the "doOpen" function within NppIO.cpp) and replaces the directory path with "\".

I'm not sure if we would be able to create the file given that we no longer have the path. Would an appropriate way to handle this be to check for \\?\ in the filename and give an error message?

@xomx
Copy link
Contributor

xomx commented Nov 30, 2022

@nickdavi

Right now I just have the message box displaying "{filename} cannot be opened." Is this acceptable? or is there a better way to handle this?

Hello, thanks for trying to help, I appreciate that.

Your solution is better than nothing. I had exactly this in my mind before but as I am a curious person, I gave it a try this week and tried some simple N++ patch to allow even such file to be opened/edited/saved. Surprisingly - it was easy (to some extent), you can try this x64 debug binary if you like:

  • new msgbox at the "\?\C:\TEMP\NPP\file." file opening by notepad++.exe \\?\C:\TEMP\NPP\file.:
    new_warning_msgbox
  • such file really opened in the N++after clicking on the OK above, magnified screenshot of the N++ tab:
    successfully_opened_file
  • as such it can be now edited and saved (like Ctrl+S etc, but not by SaveAs)
  • some other things, like the RecentFiles list showing & reopening from there, works ok:
    recent_files_ok
  • but the menu File -> Open... does not work:
    file_open_bad

There I stopped because of the N++ uses there new WinVista+ COM IFileOpenDialog interface and not the standard WIN32 WINAPI. I probably see what is going wrong there but did not have more time to continue this week.

So if you like to do a simple PR like you mentioned above (detecting of such file opening, showing a MB_OK messagebox about this to the user & abort such file-loading), you can continue ahead for sure. I do not know when or if I even finish such advanced patch, so it is better to have at least N++ without the crash. And I do not mention that Don maybe will not like such advanced non-simple solution at all ;-)

@white-gthb
Copy link
Author

The objective of this report is to prevent the crash. A displayed error message is a plus (nice to have). I feel an advanced patch is beyond the scope of this report and is more a feature request for better support for the \\?\ prefix.

My suggestion is to fix the crash first. Enhancements may or may not be added later.

@xomx
Copy link
Contributor

xomx commented Dec 2, 2022

@nickdavi

I see that your PR was rejected. I expected so (I hoped that somebody else will solve this issue, but at the end I spent more of my time writing my review of your PR that if I do such simple PR myself). But because of I like people who give (trying to contribute here code, bug reports, advice etc...) and not just only take, I will spend some more time and words below.

If you feel that the rejection was too strict, please don't be disappointed. Rather try to look at this situation from the point of view of the current N++ project maintainer. There are many N++ users and a lot of possible contributors. But only one decision maker. He has to keep this project (now rather huge!) going. For that he has to play something like the simultaneous chess board game with all the other contributors. This is very time & resource consuming stuff. So he doesn't have to, and sometimes he simply can't explain (because of his limited time slice), why he wants to do this or that.

It simply amazes me how much enthusiasm & stamina he is able to give to this project for free. Even if I take into account that he probably enjoys coding and interaction with other people. I say it here also because of I saw that people are sometimes disillusioned from some of his decisions. Even if they can be right in some cases, he has every right to do what he wants.

Enough talking. I will try to take care of this issue during this weekend. I have an idea how to solve this a little bit better without too much another coding effort from my side.

@nickdavi
Copy link

nickdavi commented Dec 2, 2022

Thank you for taking the time to write that response I really do appreciate it. I do not want to take up any more time but after your initial feedback, I had written more code that should solve the problem in just a few lines of code which I would be happy to submit a PR for if it would save you some time. I have attached a screenshot.
Open_Source_Img

@donho
Copy link
Member

donho commented Dec 3, 2022

@xomx
Thank you the explanation from your side for me.

@nickdavi
The PR was rejected because the provided solution is not generic - that means there might be another similar case which makes crash again.

@xomx
The problem is indeed due to globbing variable. Have you any global solution ?

but the crash wild recursion here is caused by N++ incorrect treating of the \?\ filename-heading as a globbing

What's the correct treatment for such name?

@xomx
Copy link
Contributor

xomx commented Dec 5, 2022

@donho

The problem is indeed due to globbing variable. Have you any global solution ?

Yes, I think so.

What's the correct treatment for such name?

N++ has to use a raw filename as it is. It means no filename string translations like a possible shortcut resolving or using an expansion by the GetFullPathName WINAPI on it, rule N++ globbing out, etc.

What I also have in my mind here is a possible future N++ support of filenames longer than the MAX_PATH relic. Using these filenames with the \\?\... (or \\?\UNC\...) prefix is the way to go for that (other possibility is to switch ON the long paths globally, it is the opt-in in Windows 10 version 1607+).

For now I am considering only raw filenames with these restrictions (otherwise it brings N++ plugins incompatibility):

  • maximum such filename length is still the MAX_PATH
  • excluding (temporarily) any unconforming Win32-filenames like these with 'space' or 'dot' char(s) at its end and some others

And any other raw filename is (for now) reported with the help of the already existing "OpenFileError" N++ messageBox and the opening of such file is immediately aborted.

I will create a PR soon, where you will be able to consider its complexity versus its benefits. So far my patched N+ works ok for me, I see no restrictions or incorrect behavior inside N++. And we can always return to a simpler solution if you will not like it.

@white-gthb
Copy link
Author

rule N++ globbing out

I'm not sure about this one. AFAIK Windows APIs still interpret wildcards when using the \\?\ namespace.
But the question mark in the \\?\ prefix itself should of course not be interpreted as a wildcard.

@xomx
Copy link
Contributor

xomx commented Dec 5, 2022

@white-gthb

Do you have a link to some MS documentation about this? I could not find any.

What I tried:

  • create a set of files (file1.txt, file2.txt and file3.txt) in C:\TEMP\
  • 1st, try to open a single raw filename by: "notepad.exe \\?\C:\TEMP\file1.txt" ... ok, it is working
  • 2nd, try a globbing by "notepad.exe \\?\C:\TEMP\file?.txt" or "notepad.exe \\?\C:\TEMP\file*.txt" ... error, not working:
    WinNotepad_globbing_attempt

Anyway, I am not going to support globbing for raw filenames in N++ now because of in such case I have to rewrite the otherwise working N++ globbing code part.

Edit:
Ah, probably a bad example, as the Windows Notepad seems to not support such globbing at all - the same error message I have got for the "notepad.exe C:\TEMP\file?.txt"...

@white-gthb
Copy link
Author

@xomx
No, I don't know any link to documentation.

The error shown in your example, shows that wildcards are interpreted and considered to be invalid.

I'm not able to test it myself but I think the API functions FindFirstFileW and FindFirstFileExW work with \\?\ prefix and using wildcards. So the parsing of wildcards still occurs when the \\?\ prefix is used.

The Microsoft documentation you mentioned says:

For file I/O, the "\\?\" prefix to a path string tells the Windows APIs to disable all string parsing and to send the string that follows it straight to the file system.

But I don't think you should interpret "disable all string parsing" too literally.

@donho
Copy link
Member

donho commented Dec 6, 2022

@xomx
Could you elaborate the term "raw filename" please?

@xomx
Copy link
Contributor

xomx commented Dec 6, 2022

@white-gthb

I think the API functions FindFirstFileW and FindFirstFileExW work with \?\ prefix and using wildcards. So the parsing of wildcards still occurs when the \?\ prefix is used.

You are right. Nowadays I am overloaded with work but I found a time to test your hint. So thanks for your push - now my N++ also works with the globbing & raw filenames together ;-) E.g.: "\\?\C:\TEMP\file?.txt", "\\?\C:\TEMP\file*.txt" and also the dir-only-variant "\\?\C:\TEMP\" open all the files intended. N++ globbing code simply proved to be ready even for this variant, I had to change only one source code line there. Both the C/C++ and the WINAPI low-level IO funcs used in N++ do not have problem with the raw filenames.

@xomx
Copy link
Contributor

xomx commented Dec 6, 2022

@donho

Could you elaborate the term "raw filename" please?

By the "raw" I mean here filenames escaped by the "\\?\" (or \\?\UNC\) prefix. These, for which is all the usual WINAPI/shell string parsing, checking and automatic path expansion disabled. (Ref.)

Edit:
There is also one similar prefix "\??\", but this is used by the NT Object Manager for searching in the caller's local device directory.

@donho
Copy link
Member

donho commented Dec 6, 2022

@xomx
Thank you for the ref, very useful.
Could you give me a usage example to use \\?\ or \\?\UNC\ under Windows 11? Via CMD or explorer or any application please?

@xomx
Copy link
Contributor

xomx commented Dec 6, 2022

@donho

Run the cmd and type there:

notepad.exe \\?\your_existing_filename_with_absolute_path
notepad.exe \\?\UNC\servername\sharename\path\filename

e.g.

notepad.exe \\?\C:\Windows\win.ini
notepad.exe \\?\UNC\name-of-your-PC\admin$\win.ini

@white-gthb
Copy link
Author

@donho
Actually, the issue description already listed an example (see Steps to Reproduce the Issue).

Using the \\?\ namespace you can create filenames you otherwise would not be able to create. Use paths longer than MAX_PATH, create filenames with leading and trailing spaces, create filenames with trailing dot. Or, if for some reason such files exist, you can use this namespace to do operations on these files (for example type or del).

In the program Total Commander you can (for example) run the command
cd \\?\c:\
to access the file system on drive c: using the \\?\ namespace and do file/folder operations. If you set Notepad++ as external editor in Total Commander, you can edit files using Notepad++ and using the \\?\ namespace.

@donho
Copy link
Member

donho commented Dec 6, 2022

@xomx
Interesting! Thank you for the sample.
It seems the UI doesn't support Win32 namespace prefixing format.
Only under CMD can do it.
So if Win32 namespace prefixing format is for accessing the file of which the path exceeds MAX_PATH, whereas it's impossible to create such file via Windows GUI - what's the advantage of Win32 namespace prefixing format? Why does a Windows application like Notepad++ should support such format?

@xomx
Copy link
Contributor

xomx commented Dec 6, 2022

@donho

It seems the UI doesn't support Win32 namespace prefixing format.

Are you sure? I can type e.g. this to Explorer:
rawFN-Explorer

And here is my latest x64 debug N++ binary for testing.

Unlike Windows Notepad, N++ can handle globbing filenames. And the above N++ binary can now handle also the raw filenames and globbing together, you can try:
notepad++.exe "\\?\C:\Program Files\Notepad++\themes\?onokai.xml"
notepad++.exe "\\?\C:\Program Files\Notepad++\themes\M*.xml"
notepad++.exe "\\?\C:\Program Files\Notepad++\themes\"

The above binary not allows only:

  • raw filenames with lengths exceeding the MAX_PATH (until N++ and plugins are prepared for that, we cannot allow this...).
  • any nonstandard WinOS filenames (otherwise N++ COM IFileDialog based Open/SaveAs dialogs will not work ok) like:
    notepad++.exe "\\?\C:\file."
    notepad++.exe "\\?\C:\file "
    notepad++.exe \\?\C:\AUX.txt
    (also other WinOS reserved names: CON, PRN, NUL, COM1-9, LPT1-9)
  • and filenames with ASCII characters in between 0-31, <, >, | and "

@xomx
Copy link
Contributor

xomx commented Dec 6, 2022

@donho

Why does a Windows application like Notepad++ should support such format?

@white-gthb just showed you an example use in conjunction with another app:

In the program Total Commander you can (for example) run the command
cd \\?\c:\
to access the file system on drive c: using the \?\ namespace and do file/folder operations. If you set Notepad++ as external editor in Total Commander, you can edit files using Notepad++ and using the \?\ namespace.

@donho
Copy link
Member

donho commented Dec 6, 2022

@xomx @white-gthb
Thank you for your very detail explanation!

@xomx
So the fix should be quite easier right? We need only to detect the Win32 namespace prefixing format then disable the globing feature. Do you confirm?

@white-gthb
Copy link
Author

white-gthb commented Dec 7, 2022

@donho

So the fix should be quite easier right? We need only to detect the Win32 namespace prefixing format then disable the globing feature. Do you confirm?

No, because as discussed above:

The Microsoft documentation you mentioned says:

For file I/O, the "\\?\" prefix to a path string tells the Windows APIs to disable all string parsing and to send the string that follows it straight to the file system.

But I don't think you should interpret "disable all string parsing" too literally.

I think the API functions FindFirstFileW and FindFirstFileExW work with \\?\ prefix and using wildcards. So the parsing of wildcards still occurs when the \\?\ prefix is used.

You are right. Nowadays I am overloaded with work but I found a time to test your hint. So thanks for your push - now my N++ also works with the globbing & raw filenames together ;-) E.g.: "\\?\C:\TEMP\file?.txt", "\\?\C:\TEMP\file*.txt" and also the dir-only-variant "\\?\C:\TEMP" open all the files intended. N++ globbing code simply proved to be ready even for this variant, I had to change only one source code line there. Both the C/C++ and the WINAPI low-level IO funcs used in N++ do not have problem with the raw filenames.

@xomx
Copy link
Contributor

xomx commented Dec 7, 2022

@donho

So the fix should be quite easier right?

Yes, the changes needed in the N++ code are really small. I know I have promised a PR soon, sorry for the delay. I just need some free time at my devel PC to format the code a bit. I started this initially as a POC only, I had no idea that it will be so easy to do so.

We need only to detect the Win32 namespace prefixing format

Exactly.

then disable the globing feature.

Not disabling, just adjusting the existing code. Then the N++ globbing code part works as usual even with a raw variant like "\\?\C:\TEMP\file?.txt".

In the very last iteration I have found that even raw shell links can be allowed in N++, e.g. this "\\?\C:\TEMP\file.txt.lnk" is also ok from now on. Now it simply behaves like when we use usual *.lnk filename, e.g. "notepad++.exe C:\TEMP\file.txt.lnk" opens the "C:\TEMP\file.txt" in N++.

@donho
Copy link
Member

donho commented Dec 7, 2022

Yes, the changes needed in the N++ code are really small. I know I have promised a PR soon, sorry for the delay. I just need some free time at my devel PC to format the code a bit. I started this initially as a POC only, I had no idea that it will be so easy to do so.

@xomx
No rush at all, take you time - I am just trying to estimate the complexity of solution - to make sure there's no regression.
:)

@white-gthb
Thank you for your reply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants