Skip to content
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

Merged
merged 1 commit into from
Aug 13, 2018

Conversation

adriansr
Copy link
Contributor

@adriansr adriansr commented Aug 2, 2018

  • ntdll.NtQueryInformationProcess (internal/unsupported API!)
    PROCESS_BASIC_INFORMATION struct
    UNICODE_STRING struct
    RTL_USER_PROCESS_PARAMETERS struct
  • kernel32.ReadProcessMemory
  • psapi.GetProcessImageFileNameA
  • psapi.EnumProcesses

@adriansr adriansr added the enhancement New feature or request label Aug 2, 2018
Copy link
Contributor Author

@adriansr adriansr left a 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

@adriansr adriansr force-pushed the feature/process_info branch 3 times, most recently from c66d478 to 5f20f19 Compare August 2, 2018 11:27
Copy link
Member

@andrewkroh andrewkroh left a 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
Copy link
Member

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.

Copy link
Contributor Author

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
Copy link
Member

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.
Copy link
Member

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?

Copy link
Contributor Author

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.
Copy link
Member

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
Copy link
Member

@andrewkroh andrewkroh Aug 3, 2018

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 {
Copy link
Member

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 {
Copy link
Member

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.

Copy link
Member

@andrewkroh andrewkroh Aug 3, 2018

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:

Copy link
Contributor Author

@adriansr adriansr Aug 3, 2018

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

Copy link
Contributor Author

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)

Copy link
Member

@andrewkroh andrewkroh left a 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
@adriansr
Copy link
Contributor Author

Added CHANGELOG and squashed

@andrewkroh andrewkroh merged commit a4ab469 into elastic:master Aug 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants