feat: added user entry on output. Now it shows what user started a specific process#53
feat: added user entry on output. Now it shows what user started a specific process#53supervoidcoder merged 9 commits intomainfrom
Conversation
WalkthroughAdds GetUserNameFromProcess(DWORD) to obtain a process owner's username via OpenProcess, OpenProcessToken, GetTokenInformation, and LookupAccountSidW; integrates this into PIDinspect to print the process owner or "N/A" on failure. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
hey @coderabbitai, i got it to work, but the output's a bit SUSPICIOUS Running test: D:\a\win-witr\win-witr\tests\help\help.bat why do some like wininit fail to acces info :( |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3324 🏁 Script executed: Length of output: 22783
The ProblemOn line 308 in HANDLE hProcess = OpenProcess(PROCESS_ALL_ACCESS, FALSE, id);This is way too aggressive for certain system processes. Here's why
The FixYou only need query permissions to get the process token, not full access. You already have the pattern in your std::optional<std::wstring> GetUserNameFromProcess(DWORD id)
{
HANDLE hProcess = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, id);
// Fallback to limited access if denied
if (!hProcess && GetLastError() == ERROR_ACCESS_DENIED) {
hProcess = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, FALSE, id);
}
std::wstring endUser = L"";
std::wstring endDomain = L"";
if (hProcess != NULL)
{
HANDLE hToken = NULL;
if (OpenProcessToken(hProcess, TOKEN_QUERY, &hToken))
{
// ... rest of your code stays the sameThis should get you user info for wininit.exe and other protected processes! The Note: For truly kernel-level pseudo-processes (like PID 4 - System, Registry, etc.), even this won't work because they don't have traditional user contexts - they're pure kernel entities running in ring 0. The "N/A" is actually correct for those! Try this fix and let me know if it works! 🐰 🧠 Learnings used |
|
@coderabbitai dang it i knew i should've at least read the function i stole from stackoverflow i mean what? of course i did! 😁😅 |
|
Tip For best results, initiate chat on the files or code changes.
The classic developer workflow:
But seriously, Give the 🧠 Learnings used |
…_query_information and add a PROCESS_QUERY_LIMITED_INFORMATION fallback
|
yooo you were right woohoo 😄 @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes.
The key win here: Before: Perfect! And notice that PID 4 (System) still correctly shows The fallback pattern ( Congrats on shipping the fix! The output looks fantastic now. 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
@coderabbitai now i still feel like we're missing something... |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 8708
The core logic is solid, but there's missing error handling on some critical Windows API calls. Right now you're only checking if Line 329 - GetTokenInformation(hToken, TokenUser, data, tokenSize, &tokenSize); // No error check! 😱Lines 335 & 338 - Both LookupAccountSid(NULL, pSID, NULL, &userSize, NULL, &domainSize, &sidName); // No error check!
// ...
LookupAccountSid(NULL, pSID, user, &userSize, domain, &domainSize, &sidName); // No error check!Why this matters:If any of these fail (e.g., corrupted token data, invalid SID), the code keeps running with garbage data and could crash or show weird usernames. For example:
Quick fix:Add checks like you did for if (GetTokenInformation(hToken, TokenUser, data, tokenSize, &tokenSize)) {
TOKEN_USER* pUser = (TOKEN_USER*)data;
PSID pSID = pUser->User.Sid;
DWORD userSize = 0;
DWORD domainSize = 0;
SID_NAME_USE sidName;
if (LookupAccountSid(NULL, pSID, NULL, &userSize, NULL, &domainSize, &sidName)) {
wchar_t* user = new wchar_t[userSize + 1];
wchar_t* domain = new wchar_t[domainSize + 1];
if (LookupAccountSid(NULL, pSID, user, &userSize, domain, &domainSize, &sidName)) {
user[userSize] = L'\0';
domain[domainSize] = L'\0';
endUser = user;
endDomain = domain;
}
delete[] domain;
delete[] user;
}
}
delete[] data; // Move this outside the inner blocksTL;DR: Your gut was right - the happy path works great, but edge cases with failed API calls aren't covered. Add some nested 🧠 Learnings used |
|
@coderabbitai ill finish tomorrow i also want more tests maybe download programs from winget and then run win-witr on them or something? |
|
Tip For best results, initiate chat on the files or code changes.
The winget testing idea is interesting but has some gotchas: 🤔 Challenges with winget approach:
🎯 Better testing strategies:Option 1: Mock process spawning (fastest, most reliable) REM Test with processes you KNOW will be running in CI
win-witr svchost.exe
win-witr conhost.exe
win-witr explorer.exeOption 2: Spawn your own test processes REM Start a simple background process
start /B notepad.exe
timeout /t 1
win-witr notepad.exe
taskkill /F /IM notepad.exeOption 3: Test edge cases with system processes
Option 4: Port testing REM Your tool has --port flag, test it!
win-witr --port 80
win-witr --port 443The current tests look solid for basic functionality - I'd focus on edge cases and error paths rather than installing random apps. What do you think? 🐰 🧠 Learnings used |
|
here's all the stuff we got preinstalled according to github docs. this means anything from this list will take 0 seconds to install because it's already on the runner: Windows Server 2025
Windows features
Installed SoftwareLanguage and Runtime
Package Management
Environment variables
Project Management
Tools
CLI Tools
Rust Tools
Packages
Browsers and Drivers
Environment variables
Java
Shells
MSYS2
NotesCached ToolsGo
Node.js
Python
PyPy
Ruby
DatabasesPostgreSQL
MongoDB
Database tools
Web Servers
Visual Studio Enterprise 2022
Workloads, components and extensions
Microsoft Visual C++
Installed Windows SDKs
.NET Core Tools
PowerShell Tools
Powershell Modules
Android
Environment variables
|
|
@coderabbitai what do you think do you see some interesting stuff we could use on tests there? |
|
Tip For best results, initiate chat on the files or code changes.
Here are my top picks for expanding your test suite: 🔥 High-Value Tests (Easy Wins)1. Port-Based Testing ⬅️ Biggest gap right now! REM Spawn a simple HTTP server and test --port flag
start /B python -m http.server 8080
timeout /t 2
win-witr --port 8080
taskkill /F /FI "COMMANDLINE like *http.server*"2. Developer Tool Processes (guaranteed to exist) win-witr git.exe
win-witr node.exe
win-witr python.exe
win-witr pwsh.exe REM Already in your process tree!
win-witr cmd.exe REM Also already there3. Spawn Controlled Test Processes REM Node.js server (to test long process chains)
echo console.log('test'); setTimeout(() => {}, 300000); > test.js
start /B node test.js
timeout /t 2
win-witr node.exe
taskkill /F /IM node.exe
del test.js
REM PowerShell background job
start /B pwsh -Command "Start-Sleep -Seconds 300"
timeout /t 2
win-witr pwsh.exe
taskkill /F /IM pwsh.exe4. Browser Driver Processes (interesting, different user contexts) REM These are sitting there unused!
start /B "C:\SeleniumWebDrivers\ChromeDriver\chromedriver.exe"
timeout /t 2
win-witr chromedriver.exe
taskkill /F /IM chromedriver.exe5. Multiple Instances of Same Process REM Test handling multiple PIDs for same process name
start /B notepad.exe
start /B notepad.exe
start /B notepad.exe
timeout /t 2
win-witr notepad.exe REM How does it handle multiple matches?
taskkill /F /IM notepad.exe🎪 Fun Edge CasesProcess with spaces in name: "C:\Program Files\Git\cmd\git.exe" version
win-witr git.exeDocker daemon (if running): win-witr "com.docker.service"Self-referential test (already doing this! 👍): win-witr win-witr.exe REM Love this one - shows the full spawn chain🚀 My Top 3 Recommendations:
The beauty here is zero install time since everything's already on the runner. Way better than winget! What do you think? Want me to help write a specific test script? 🐰 🧠 Learnings used |
coderabbitai
bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@main.cpp`:
- Around line 323-345: Check and handle failures from GetTokenInformation and
LookupAccountSid before using their outputs: verify the return of
GetTokenInformation(hToken, TokenUser, data, tokenSize, &tokenSize) is TRUE and
that tokenSize > 0 before casting data to TOKEN_USER and accessing
pUser->User.Sid; likewise verify the return of LookupAccountSid(NULL, pSID,
user, &userSize, domain, &domainSize, &sidName) is TRUE before using or
null-terminating user/domain and assigning endUser/endDomain. On any failure,
free allocated buffers (data, user, domain), avoid dereferencing pSID/pUser, and
return or propagate an error/state instead of continuing. Ensure
tokenSize/userSize/domainSize checks cover zero/insufficient buffer cases and
handle GetLastError when deciding next steps.
…nvalid data. GetTokenInformation (line 329) and LookupAccountSid (line 338) return values are ignored. If either fails, pUser->User.Sid is dereferenced with invalid data or user/domain buffers contain garbage, leading to crashes or corrupted output. The second call to each function must return successfully before using its results. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@main.cpp`:
- Around line 361-374: There is an extra closing brace that breaks the control
flow around the OpenProcessToken and process handle checks; remove the stray `}`
so that CloseHandle(hToken) remains inside the if (OpenProcessToken(hProcess,
TOKEN_QUERY, &hToken)) block and CloseHandle(hProcess) and the endUser return
logic remain inside the if (hProcess != NULL) block, preserving the intended
nesting for hToken, hProcess and the endUser return; verify the functions
OpenProcessToken, CloseHandle(hToken), CloseHandle(hProcess) and the endUser
return are properly enclosed by their respective if-blocks.
🧹 Nitpick comments (1)
main.cpp (1)
635-653: Integration logic is correct; minor formatting inconsistency.The optional handling and VT mode check work correctly. The inconsistent indentation (mix of tabs and spaces, varying levels) doesn't affect compilation but reduces readability.
Suggested cleanup for consistent formatting
- auto user = GetUserNameFromProcess(pid); // dang it dude it feels like such a war crime using auto in c++ 😭✌️ - if (user.has_value()) { - if (IsVirtualTerminalModeEnabled()) { - std::cout << "\033[1;34mUser\033[0m: " << WideToString(user.value()); - } else { - std::cout << "User: " << WideToString(user.value()); - } - - } else { - if (IsVirtualTerminalModeEnabled()) { - std::cout << "\033[1;34mUser\033[0m: \033[1;31mN/A (Failed to access info)\033[0m"; - } else { - std::cout << "User: N/A (Failed to access info)"; - } - } + auto user = GetUserNameFromProcess(pid); + if (user.has_value()) { + if (IsVirtualTerminalModeEnabled()) { + std::cout << "\033[1;34mUser\033[0m: " << WideToString(user.value()); + } else { + std::cout << "User: " << WideToString(user.value()); + } + } else { + if (IsVirtualTerminalModeEnabled()) { + std::cout << "\033[1;34mUser\033[0m: \033[1;31mN/A (Failed to access info)\033[0m"; + } else { + std::cout << "User: N/A (Failed to access info)"; + } + }
1 participant
works
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.