Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

pradishmp
Copy link
Contributor

Fixes #8351

What changed ?
This is a corner case scenario where there is a potential for a memory leak. For this leak to occur, two things must happen:

  1. The arguments passed to the service should be wide char strings.
  2. The function call toMBString, which is used to convert the wide char strings to multibyte strings, should fail, returning a nullptr.

When a nullptr is returned, as part of cleaning up the heap memory allocated, the cleanArgs() function is called. However, this function will only clean up the memory if the Boolean variable owns_argv_ptrs is set to true. In this case, as the variable is still false, the function will only remove all elements from the vector.

Later, when the Boolean variable owns_argv_ptrs is set to true and the destructor gets a chance to clean up, it cannot do anything as the elements are already removed and the vector size is zero.

Therefore, the fix is to set the owns_argv_ptrs_ = true; immediately after the CommandLineToArgvW returns successfully, thus incorporating the code review comment give by michael-myers this will speed up the memory clean up process, rather than wait for the destructor to do the job.

@pradishmp pradishmp requested review from a team as code owners July 4, 2024 16:48
@pradishmp
Copy link
Contributor Author

my changes are specific to windows platform, build and unit test cases pass on my PC, can anyone who had commited before do a quick check.

@directionless
Copy link
Member

I'm not sure what's up with CI. But it sounded like we can push this to the next release

@Smjert
Copy link
Member

Smjert commented Jul 6, 2024

Closing and reopening to get latest updates from CI

@Smjert Smjert closed this Jul 6, 2024
@Smjert Smjert reopened this Jul 6, 2024
@directionless
Copy link
Member

(There's a little discussion about this in #8367)

@directionless directionless merged commit 02a883d into osquery:master Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Potential memory leak in class ServiceArgumentParser's Constructor

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.