#656 Fixed many many many bugs.

[Alexander Riccio]

I fixed a very large number of issues, from buffer overruns to format string errors. This should be a no-brainer.

[Don HO]

AFAIK, quite a lot. And some of my friends still use it.
And the policy of Notepad++ is to try to keep the compatibility with the old OS - that's why the project sticked on VS2005 for a long time.

[Alexander Riccio]

Ahh ok. That's a perfectly valid reason. I'll have to see how I can work around the lack of proper string handling functions.

For the record: I'm 100% for dropping support for XP as soon as possible.

I'm curious: What's their reasoning? It doesn't even get security updates anymore!

[Alexander Riccio]

Actually I was wrong that vc120_xp "doesn't support any of them", it supports all of the NON strsafe bounds checked functions.

[Alexander Riccio]

Change summary:
1. Fixed operator precedence confusion for ::CheckMenuItem in Notepad_plus::command(int id)
(was that me?)
2. Fixed format specifier type mismatches in Notepad_plus::command(int id)
3. Fixed incorrect usage of GetFullPathName/GetLongPathName in Notepad_plus::doOpen
4. Fixed GetClassName Unicode build buffer overrun in Notepad_plus::notify
5. Fixed 2 invalid uses of failed SHGetSpecialFolderLocation's Out parameters in NppParameters::getCloudSettings
6. Fixed an invalid use of failed SHGetSpecialFolderLocation's Out parameters in NppParameters::getSetting
7. Fixed an invalid use of failed SHGetSpecialFolderLocation's Out parameters in NppParameters::load()
8. Fixed wsprintf format specification type mismatch in Date::toString
9. Fixed incorrect usage of GetFullPathName in checkSingleFile
10. When I was looking through the svn revision, I noticed that all of the multibytetowidechar buffer ovverruns hadn't been merged.

[Alexander Riccio]

Grr. Posting from my phone is very frustrating.

[Andreas Jonsson]

I don't think the changes (rev 1352) in Notepad_plus::doOpen interact well with file system redirection. I tried to open C:\windows\system32\drivers\etc\hosts but it failed because GetLongPathName returns 0 (GetLastError() = ERROR_FILE_NOT_FOUND).

[Alexander Riccio]

For reference: http://svn.tuxfamily.org/viewvc.cgi/notepadplus_repository/trunk/PowerEditor/src/NppIO.cpp?r1=1351&r2=1352&pathrev=1352&diff_format=u

Not sure what's up, but it sounds like we've exposed a bug, not introduced one.

[Alexander Riccio]

It'd probably be a good idea to log something on doOpen's failure.

[Alexander Riccio]

I assume this worked before 1352?

[Andreas Jonsson]

Yes, it worked before.

[Alexander Riccio]

Can you tell me more about that file? Attributes? Contents? Can you open it in windows standard Notepad? The OS you're using?

[Andreas Jonsson]

https://msdn.microsoft.com/en-us/library/windows/desktop/aa384187%28v=vs.85%29.aspx

It's not really about that file itself, but more about the system32 directory. Since N++ is a 32 bit application, Windows will try to forward any file accesses in system32 to syswow64.

So when you try to access system32\drivers\etc\hosts it will forward you to syswow64\drivers\etc\hosts. Since that file doesn't exist, GetLongPathName will fail with ERROR_FILE_NOT_FOUND.

N++ used to be able to handle this by turning off file system redirection if the file didn't exist (or rather: if Windows claimed the file didn't exist). But if one insists on checking the return value of GetLongPathName, the function which turns off redirection (safeWow64EnableWow64FsRedirection) will never be reached:

    const DWORD getLongPathNameResult = ::GetLongPathName(longFileName, longFileName, longFileNameBufferSize);
    if ( getLongPathNameResult == 0 )
    {
        return BUFFER_INVALID;
    }

...

    bool isWow64Off = false;
    if (!PathFileExists(longFileName))
    {
        pNppParam->safeWow64EnableWow64FsRedirection(FALSE);
        isWow64Off = true;
    }

[Alexander Riccio]

*AHHHHH this all makes sense now!*

[Alexander Riccio]

doOpen needs to be totally refactored, it's too fragile - fixing bugs breaks legitimate uses.

[Alexander Riccio]

I'm away from home until the 25th, so I can't fix it immediately, but we need to figure out:

1. How is doOpen used?
2. What code relies on FS-redirection?
3. Where is it called from?
4. Can we plausibly change every single use of doOpen?

[Alexander Riccio]

As a side note: MAX_PATH isn't actually that maximum size of an NTFS path, but the maximum size of a single component. ~33,000 is the max size of an NTFS path.

[Alexander Riccio]

I'm sorry to be pushy/impatient, but could someone grep for doOpen?

[Alexander Riccio]

This is kinda funny, I actually ran into a similar issue in doOpen, almost a year ago, as reported in bug #4945, but totally forgot about it. I'm also thinking that it's more exploitable than I originally thought.

That should be PLENTY of evidence that doOpen needs complete refactoring.

[Don HO]

Thank you Adrea
This regression is fixed in notepad++ github repo now:
https://github.com/donho/notepad-plus-plus

Don

[Alexander Riccio]

WAIT A SECOND THERE'S A GITHUB REPO??!??

Grr. Do you accept PR's on GitHub?

Why didn't I know about this ;)

[Don HO]

github repo was created few days ago.
And the push request is not accepted on github.
I will find a better way to do that.

[Alexander Riccio]

Ah, ok, cool.

I'm just happy to be able to contribute :)

[Alexander Riccio]

Here's the proper, TortoiseSVN compatible, patch.

[Andreas Jonsson]

Possible regression due to this patch: http://sourceforge.net/p/notepad-plus/bugs/5266/