[GITHUB-1042] c.s.j.p.WindowUtils.W32WindowUtils.getProcessFilePath does not close process handle #1043
Conversation
…oes not close process handle
matthiasblaesing
force-pushed
the
github-1042
branch
from
895346d to
2933a8b
Compare
| if(Kernel32.INSTANCE.GetLastError() != WinNT.ERROR_ACCESS_DENIED) { | ||
| throw new Win32Exception(Kernel32.INSTANCE.GetLastError()); | ||
| } else { | ||
| // Ignore windows, that can't be accessed |
There was a problem hiding this comment.
This seems to swallow any other unexpected error and return an incorrect result! Also I don't understand the comment.
There was a problem hiding this comment.
No it does not. Line 1290 is entered if GetLastError is not ERROR_ACCESS_DENIED, so any other error is raised as a Win32Exception. Before and after the change trying to access a process we don't have access to, will return the empty string.
| throw new Win32Exception(Kernel32.INSTANCE.GetLastError()); | ||
| } else { | ||
| // ignore invalid handles | ||
| return ""; |
There was a problem hiding this comment.
Same here. The exception should be thrown on any other error, the previous code was correct IMO.
There was a problem hiding this comment.
See my previous reply. What is important: calling Native.toString(filePath).trim() on an initialized char[] (all \0) will result in the empty string.
There was a problem hiding this comment.
Makes sense, thanks. Maybe I should question the original implementation?
There was a problem hiding this comment.
You mean, that the function(s) should not return the empty string in case of an error? Yeah, I thought about "fixing" that, but I decided against it, as it basically became API by convention.
There are other thinks I would change looking at it - seeing a HWND in a common class is suspicious.
…