-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add APIs to fetch processes information #6
Conversation
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.
Do not merge yet
Please review
c66d478
to
5f20f19
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.
Could you add some basic tests for the new functions?
kernel32.go
Outdated
@@ -33,6 +33,7 @@ import ( | |||
//sys _GetTickCount64() (millis uint64, err error) = kernel32.GetTickCount64 | |||
//sys _GetSystemTimes(idleTime *syscall.Filetime, kernelTime *syscall.Filetime, userTime *syscall.Filetime) (err error) = kernel32.GetSystemTimes | |||
//sys _GlobalMemoryStatusEx(buffer *MemoryStatusEx) (err error) = kernel32.GlobalMemoryStatusEx | |||
//sys _ReadProcessMemory(handle syscall.Handle, baseAddress uintptr, buffer uintptr, size uintptr, numRead *uintptr) (s bool) = kernel32.ReadProcessMemory |
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.
Rather than returning a bool, why not return err error
and pass that back to the caller? I think this would be more meaning than ErrReadFailed.
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.
👍
@@ -67,6 +68,9 @@ const ( | |||
ProcessorArchitectureUnknown ProcessorArchitecture = 0xFFFF | |||
) | |||
|
|||
// ErrReadFailed is returned by ReadProcessMemory on failure |
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.
nit: add a period to end the sentence.
|
||
// Syscalls | ||
// Warning: NtQueryInformationProcess is an unsupported API that can change | ||
// in future versions of Windows. Available from XP to Windows 10. |
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.
Does this work on Win 2016?
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.
Yes, that's one of the systems I used for testing. It works in XP and everything after it.
|
||
// NtQueryInformationProcess is a wrapper for ntdll.NtQueryInformationProcess. | ||
// The handle must have the PROCESS_QUERY_INFORMATION access right. | ||
// Returns an error that can be cast to a NTStatus type. |
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 think this is just semantics, but how about directly stating that the error type is NTStatus
.
const ( | ||
// PROCESS_BASIC_INFORMATION ProcessInformationClass parameter for | ||
// NtQueryInformationProcess. | ||
ProcessBasicInformation = 0 |
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.
WDYT about making a type ProcessInformationClass uint32
and define constants for the known values?
I think this would make the infoClass
param in NtQueryInformationProcess
slightly more clear.
// NtQueryInformationProcess is a wrapper for ntdll.NtQueryInformationProcess. | ||
// The handle must have the PROCESS_QUERY_INFORMATION access right. | ||
// Returns an error that can be cast to a NTStatus type. | ||
func NtQueryInformationProcess(handle syscall.Handle, infoClass uint32, info unsafe.Pointer, infoLen uint32, returnLen *uint32) error { |
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'd prefer having the function return the returnLen
rather than using an out param.
// RtlUserProcessParameters is Go's equivalent for the | ||
// _RTL_USER_PROCESS_PARAMETERS struct. | ||
// A few undocumented fields are exposed. | ||
type RtlUserProcessParameters struct { |
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.
How / where / for what will this be used? Just curious.
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 passed to NtQueryInformationProcess depending on the OS version? :guessing:
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.
Get the command-line and cwd for a running process, in go-sysinfo
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.
So, to make it clear, to get the command-line arguments for another process:
Get process handle -> NtQueryInformationProcess(PROCESS_BASIC_INFORMATION) -> ReadProcessMemory() to read PEB -> ReadProcessMemory() to read RtlUserProcessParameters (pointer in PEB)
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.
The latest changes LGTM. There are a few linter errors that are failing the build. I think they are all easily correctable, but if not we can add an exception and ignore them.
- ntdll.NtQueryInformationProcess (internal/unsupported API!) PROCESS_BASIC_INFORMATION struct UNICODE_STRING struct RTL_USER_PROCESS_PARAMETERS struct - kernel32.ReadProcessMemory - psapi.GetProcessImageFileNameA - psapi.EnumProcesses
859aef3
to
8d35ed7
Compare
Added CHANGELOG and squashed |
PROCESS_BASIC_INFORMATION struct
UNICODE_STRING struct
RTL_USER_PROCESS_PARAMETERS struct