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

Release variable in windows data conversation #7024

Merged
merged 4 commits into from
Mar 28, 2021

Conversation

directionless
Copy link
Member

@directionless directionless commented Mar 26, 2021

Release variable in cimDatetimeToUnixtime

i think I got all the returns. It passes CI. I can download and run the resultant osqueryd. Not sure if there's much else to manually test here

Fixes: #7018

Release variable in `cimDatetimeToUnixtime`

i think I got all the returns

Fixes: osquery#7018
Copy link
Member

@zwass zwass left a comment

Choose a reason for hiding this comment

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

Can we use the scope_guard strategy for this? As seen in https://github.com/osquery/osquery/pull/7024/files#diff-572b7f350484fd1cc8be8e82ef3948646c04e248e249ef6c0a2a1b7dda592b06R103-R104.

That helps us ensure that the Release() is called even if more code paths are added later.

osquery/utils/conversions/windows/strings.cpp Outdated Show resolved Hide resolved
@directionless
Copy link
Member Author

directionless commented Mar 27, 2021

Can we use the scope_guard strategy for this?

That sounds much cleaner. I was wishing for a go style defer, let's see if CI says I got the syntax right

Copy link
Member

@zwass zwass left a comment

Choose a reason for hiding this comment

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

Yes scope guard is awesome! Very similar to Go's defer, though cool that it works on a scope basis with RAII.

osquery/utils/conversions/windows/strings.cpp Outdated Show resolved Hide resolved
zwass
zwass previously approved these changes Mar 27, 2021
Copy link
Member

@zwass zwass left a comment

Choose a reason for hiding this comment

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

Docs (https://docs.microsoft.com/en-us/windows/win32/api/combaseapi/nf-combaseapi-cocreateinstance) seem to indicate we should be good with this.

Upon failure, *ppv contains NULL

@directionless
Copy link
Member Author

I downloaded the CI artifact and ran it on windows, select * from processes seemed to work.

@directionless
Copy link
Member Author

CI artifact still seems okay for select * from processes which I think his this code path.

Kinda makes me wondering about sprinkling this scope_guard release pattern everywhere

@directionless directionless merged commit 91ad4bc into osquery:master Mar 28, 2021
@directionless directionless deleted the seph/issue-7018 branch March 28, 2021 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

leak in cimDatetimeToUnixtime
3 participants