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.

Sign up for GitHub

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

Jump to bottom

[windows] - Fix permissions for GetProcessMemoryInfo #185

Merged
merged 3 commits into from
Dec 13, 2024
Merged

[windows] - Fix permissions for GetProcessMemoryInfo #185

merged 3 commits into from
Dec 13, 2024

Conversation

VihasMakwana
Copy link

@VihasMakwana VihasMakwana commented Dec 9, 2024 •
edited
Loading

As per https://learn.microsoft.com/en-us/windows/win32/api/psapi/nf-psapi-getprocessmemoryinfo, GetProcessMemoryInfo doesn't need windows.PROCESS_VM_READ. Hence, remove it and only use windows.PROCESS_QUERY_LIMITED_INFORMATION

Related elastic/beats#41407

@VihasMakwana VihasMakwana self-assigned this Dec 9, 2024
gabriellandau
gabriellandau approved these changes Dec 9, 2024
View reviewed changes
Copy link

@gabriellandau gabriellandau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix

gabriellandau
gabriellandau reviewed Dec 9, 2024
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@VihasMakwana VihasMakwana requested a review from v1v December 10, 2024 10:45
andrewkroh
andrewkroh reviewed Dec 10, 2024
View reviewed changes
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.

LGTM. But I think we need to fix the CI issues in a separate PR before merging this.

@VihasMakwana
Copy link
Author

VihasMakwana commented Dec 11, 2024 •
edited
Loading

@andrewkroh I'll open another PR to fix CI for this repo.

@VihasMakwana
Copy link
Author

@andrewkroh can you please review #187? It should fix the CI

@VihasMakwana
Copy link
Author

@andrewkroh CI looks good, can you please take a look when you have moment (and possibly merge it if looks good 😅)?

@andrewkroh andrewkroh merged commit 73e74b5 into elastic:master Dec 13, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabriellandau gabriellandau gabriellandau approved these changes

@andrewkroh andrewkroh andrewkroh approved these changes

@v1v v1v Awaiting requested review from v1v

Assignees

@VihasMakwana VihasMakwana

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@VihasMakwana @andrewkroh @gabriellandau