-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Object Manager Plus #2197
base: master
Are you sure you want to change the base?
Object Manager Plus #2197
Conversation
- Standard System Informer handle properties window for Windows objects (including Directories). - Add support for missing object types: Device, EventPair, Timer, Semaphore, FilterConnectionPort - Add target link info for Symbolic links - Fix Directory handles leaking
- Add Windows Object tab to properties window: object attributes and creation time. - Add GeneralCallbackHandlePropertiesWindowPreOpen, calls on WM_INITDIALOG and allows plugins to customize general handle properties tab. -- Display real handles count and hide irrelevant access entry (EtHandlePropertiesWindowPreOpen). - Add Symbolic Link Target column - Enhanced navigation to symlink: focus to target in ListView.
- Implements search using standard SI searchbox: searches all 3 columns (name, type, symlink) - Status bar (pseudo) with fullpath to selected object (can copy on righclick menu), current directory object count - Refresh button - full rebuild of tree and current dir list - Get object address using KSI (previous commit)
- I finally understand how PH memory management works and fixed (hope) horrible previous code. - Fixed WM_KEYDOWN forwarding (both for PH and plugins), broken by 5bf7f29 - F5 - refresh, filter will be reapplied after refresh - Autosizing of Name column on window resizing. Plugin properties window position will not overwrite generic handle propertines window position setting.
…nformer into ObjManagerPlus
Awesome work! I'll help give an in-depth review when I have some cycles. Some initial feedback on the video you provided (thanks for that it's very helpful). I wonder if it makes sense to change the "Symbolic Link Target" column to a more generic "Target" column to show information/names of things more broadly? I'm thinking, for example, you could show the name for named object or the device for filter ports. Instead of only showing symbol link names. |
…nformer into ObjManagerPlus
Broken by 0c0c938
- Renamed Symbolic Link Target column to Target - Implemented (experimental) asynchronous resolver for Device and ALPC port targets. -- Resolving Device driver target. Added menu Go to device driver -- Resolving ALPC server process target. Added menu Go to server process - Rewritten navigaton to symbolic links (and device drivers)
Add target resolving for Device and ALPC port
Add target resolving for Mutant, fix search resolver_test.mp4 |
- Improve resolver. Added mutant client owner to target and menu Go to thread... - Improve filter: it now stay when changing directory. If selected entry match it stay selected and visible when performing search. - Tri-state column sort now saved and loaded from settings (ObjectManagerWindowListSort). Sort now works correctly on directory or filter change. - Enter - open properties, Shift+Enter - open security, Shift+Dblclk on symlink opens its properties.
This comment was marked as resolved.
This comment was marked as resolved.
c55653a
to
1717d3f
Compare
- TaskDialog theme on Windows 10 currently unsupported...
baaf60c
to
678539e
Compare
- Fix SymbolicLink target in handle properties
678539e
to
17ba73e
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
- Object Handles tab: added support for changing handle attributes - ET plugin extensions now available for Driver, Device, Type (globally for any handle properties windows)
850f2ac
to
318dbab
Compare
c5f930f
to
1fae386
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more things @DartVanya. I think we're very close to being able to merge 🚀 .
There is some formatting and style nits that I opted not to comment on. Mostly function close parenthesis is not correctly indented or mics style things here and there. I don't mind going to clean those up myself after merging. 👍
@@ -1363,7 +1347,7 @@ VOID PhMwpOnCommand( | |||
L"Current Window Desktop", | |||
L"Desktop", | |||
PhpOpenSecurityDesktopHandle, | |||
PhpCloseSecurityDesktopHandle, | |||
NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 is this not going to leak the handle (and below)? Also, how did this ever work? PhSecurityInformation_Release
passes the Context
to the CloseObject
and not the Handle
? Maybe it was intended for PhpOpenSecurityStationHandle
to set the output context pointer to the opened handle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle
is opened from PhStdGetObjectSecurity
via OpenObject
. After retrieving of security info by PhGetObjectSecurity
Handle
is closing by NtClose
inside PhStdGetObjectSecurity
. CloseObject
only useful to free resources after security window closes (only Object Manager uses it). Previous PhpCloseSecurityDesktopHandle
and PhpCloseSecurityStationHandle
code hadn't effect since Context
was NULL
.
If Desktop handle was open by OpenDesktop
, NtClose
can't close it and returns STATUS_HANDLE_NOT_CLOSABLE
(despite handle doesn't have the Protected attribute). But this doesn't happen if handle was duplicated (seems like it's a bug in Windows?). To fix this I added this code to PhStdGetObjectSecurity
.
if (NtClose(handle) == STATUS_HANDLE_NOT_CLOSABLE &&
PhEqualString2(this->ObjectType, L"Desktop", TRUE))
{
CloseDesktop(handle);
}
Example of huge Desktop handles leak in current release build:
s14_14-16_tD9tS.mp4
PvInitializeSettings(); | ||
PvInitializeSuperclassControls(); | ||
PhShowWarning2( | ||
GetDesktopWindow(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing this frequently in the PR, why the changes to pass the desktop window handle in these paths? It seems unnecessary given the documentation and SAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recently @dmex also asked me about this. At your discretion, I can revert this.
I changed parrent from NULL to GetDesktopWindow() to not show double icon on this dialogs. It wasn't a problem for MessageBox, since it didn't have window icon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are edits to tools/peview/searchbox.c
here, but I don't see any changes to SystemInformer/searchbox.c
? Unfortunately there is duplicated code somewhat between them. I'm not asking you do consolidate them. But I'm curious (and spot checking) that there are no changes to the complimentary file in SystemInformer/searchbox.c
.
{ | ||
if (PartId == MENU_POPUPGUTTER || PartId == MENU_POPUPBORDERS) | ||
// In Windows 11, buttons lack background, making them indistinguishable on dark backgrounds. | ||
// To address this, we invert the button. This technique isn't applicable to Windows 10 as it causes the button's border to appear chipped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking, this comment indicates that you intend to only do this on Windows 11 - but I don't see any restrictions to only Windows 11?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
V
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is commented out code and comments throughout this file indicating intention to restrict certain changes depending on the operating system version. Spot checking that this is intended as it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
V
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is commented out code and comments throughout this file indicating intention to restrict certain changes depending on the operating system version. Spot checking that this is intended as it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PhGetThemeClass and its underlying uxtheme\GetThemeClass
are only available since Win11. I tried to find some workaround, but I couldn't make a correct implementation which will support TaskDialog and doesn't break other UI parts on Windows 10. It's very frustrating why Microsoft uses same theme PartId integers for different control types 😕
In general, TaskDialog theme currently unsupported in WindowsVersion < WINDOWS_11
. Thats why I comment out this code, may be someone find solution for Win10. Currently I think what it is impossible to do without GetThemeClass.
{ | ||
KPH_ALPC_COMMUNICATION_INFORMATION connectionInfo; | ||
|
||
if (NT_SUCCESS(status = KphAlpcQueryInformation( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oddity here - this needs a check for KsiLevel()
else an assert will trigger on the uninitialized free list. I want to eventually make it so the initialization of the driver isn't a requirement for trying to call the APIs, but that's not how it works today. Please add a check if KsiLevel() >= KphLevelMed
before calling KphAlpcQueryInformation
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alpc port and driver target resolving only enabled with BOOLEAN useKsi = KsiLevel() >= KphLevelMed;
else if (entry->EtObjectType == EtObjectDevice || // allow resolving without driver for PhGetPnPDeviceName()
entry->EtObjectType == EtObjectAlpcPort && useKsi ||
entry->EtObjectType == EtObjectMutant ||
entry->EtObjectType == EtObjectJob ||
entry->EtObjectType == EtObjectDriver && useKsi)
{
entry->TargetIsResolving = TRUE;
}
In this case, resolver code will not be queued.
for (ULONG i = 0; i < Context->CurrentDirectoryList->Count; i++)
{
...
if (!entry->TargetIsResolving)
continue;
...
}
Implement properties support for Object Manager
Enhanced Object Manager
Defied WinObj: search, statusbar, refresh
[System Informer, Plugins] Fixed WM_KEYDOWN forwarding (both for PH and plugins), broken by 5bf7f29#diff-22c39c7de8c6ce2547b9cda41005993a982b12b5f4d567a14ce7e38534c2bf77L1789
Add target resolving for Device and ALPC port
Add target resolving for Mutant, fix search
Support for open symlink target in explorer
Show Job process list in target column
Add CpuPartition type support and icons for more types
New Object Handles page in properties
[phlib, kphlib] Add PhOpenDevice. And Oject Manager Plus FINAL
TreeNew: significantly improve visibility of selected item.
Support for multiple non-modal object properties windows
Add KphOpenObjectByTypeIndex