Intermittent Notepad++ crash related to NPPM_GETOPENFILENAMES (only on 64-bit NPP)

Terry R

@Mark-Olson
Sounds like you done quite a bit of testing to define the environment. The only thing I’d say is missing is conducting the same tests on another PC. Do you have ability, and of course time to set up a similar environment?

Terry

Coises

@Mark-Olson

Oy… that code is a disaster waiting to happen.

I don’t know if you can read C++: the code expects you to pass it an array of pointers to buffers; the pointers must already be set to point to the buffers, and the buffers must be large enough to contain the file name strings.

It uses lstrcpy; the Microsoft documentation for that function says, “Warning Do not use.”

I suggest pretending NPPM_GETOPENFILENAMES does not exist, and instead using NPPM_GETNBOPENFILES and NPPM_GETBUFFERIDFROMPOS to enumerate buffer ids, then NPPM_GETFULLPATHFROMBUFFERID to get the file names.

Mark Olson

@Coises

Thank you so much for identifying the badness of the NPPM_GETOPENFILENAMES code in Notepad++. 🎉 I may submit an issue in the NPP GitHub repo tomorrow.

I reimplemented the code in my plugin so that it uses almost the approach you suggested, with the difference being that I iterate over the two views and use NPPM_GETNBOPENFILES and NPPM_GETBUFFERIDFROMPOS in each view. Now there is no crash when I run the tests in 64-bit Notepad++, and they still work in 32-bit Notepad++. ~~Now to implement this fix in NppCSharpPluginPack…~~

EDIT: This issue has been fixed in the aforementioned plugins in JsonTools v8.3.1.2 and NppCSharpPluginPack v0.0.3.16.

The one wrinkle with this approach is that, if the second view is not open, the list of open file names will always have new 1 at the end, because that’s the name of the ghost placeholder buffer that a closed second view always has for mysterious internal reasons. AFAICT there’s no exposed API for determining if the second view is open, so for now I just have to accept that this is the way things are.

Alan Kilborn

@Coises said:

Oy… that code is a disaster waiting to happen

Oy…indeed. :-(

@Mark-Olson said:

AFAICT there’s no exposed API for determining if the second view is open, …

I looked up how the PythonScript plugin provides the notepad.isSingleView() function; from what I found, HERE, I would guess that you are right that Notepad++ does not directly provide an API for getting this information. :-(

…so for now I just have to accept that this is the way things are.

I suppose you could take a similar approach to what the PS plugin is doing (regard the view info)…and plow onward from there…

FWIW, HERE is how PS retrieves info via notepad.getFiles().

Coises

@Mark-Olson said in Intermittent Notepad++ crash related to NPPM_GETOPENFILENAMES (only on 64-bit NPP):

AFAICT there’s no exposed API for determining if the second view is open, so for now I just have to accept that this is the way things are.

Try NPPM_GETCURRENTDOCINDEX.

Remember that it is possible for the second view to be open and the first view closed.

Coises

@Mark-Olson said in Intermittent Notepad++ crash related to NPPM_GETOPENFILENAMES (only on 64-bit NPP):

I may submit an issue in the NPP GitHub repo tomorrow.

It hasn’t been touched for a long time. (Git Blame shows the last change as from 2015, and that appears to be just reformatting as part of a clean-up effort.)

The problem is more the interface itself than the way it is implemented. No doubt the interface has been and will be retained for compatibility with any older plugin that uses it.

I’m not sure where or how one would request this, but I suggest the appropriate course of action would be to deprecate the message. I believe that could only consist of putting note in the header file and the online documentation.

Were you able to set a breakpoint here and check whether cStrArray was as expected, and whether the crash occurred during the Notepad++ call or after it returned? I notice that the ClikeStringArray constructor uses Marshal.AllocHGlobal with a string capacity; I think you might be passing MAX_PATH to it, which is a count of characters, while AllocHGlobal is expecting bytes and the file paths are surely in Unicode.

In any case, I think MAX_PATH is not a reliable maximum on 64-bit Windows.

Mark Olson

@Coises said in Intermittent Notepad++ crash related to NPPM_GETOPENFILENAMES (only on 64-bit NPP):

Were you able to set a breakpoint here and check whether cStrArray was as expected, and whether the crash occurred during the Notepad++ call or after it returned? I notice that the ClikeStringArray constructor uses Marshal.AllocHGlobal with a string capacity; I think you might be passing MAX_PATH to it, which is a count of characters, while AllocHGlobal is expecting bytes and the file paths are surely in Unicode.

As far as I could tell, the crash seemed to be occurring during a call to Marshal.FreeHGlobal in the CLikeStringArray.Dispose method. I didn’t try to poke around in there too much, because it seemed like the method was being called asynchronously when the garbage collector collected the CLikeStringArray.

I suggest the appropriate course of action would be to deprecate the message

I agree with this. From my search for calls to lstrcpy it looked to me like there might be other plugin messages that could be deprecated.

Mark Olson

I looked at PythonScript’s approach (as @Alan-Kilborn suggested) to determining if there’s a single view, and I decided to use NPPM_GETCURRENTDOCINDEX, as @Coises suggested. It’s a little roundabout, but it feels much less hacky than the approach using FindWindowEx to me, at least in C#.

Alan Kilborn

@Mark-Olson said:

I decided to use NPPM_GETCURRENTDOCINDEX , as @Coises suggested. It’s a little roundabout, but it feels much less hacky than the approach using FindWindowEx

I agree. I don’t know why PS didn’t use that approach as well. I’d make a suggest that PS be changed, but it’s probably a case of “ain’t broke, so don’t fix it”.

Mark Olson

So it turns out that my plugin was at fault, as noted here. I was allocating MAX_PATH bytes for a buffer that could contain MAX_PATH wide chars, so any filename with length MAX_PATH / 2 or greater would cause a bug.