Skip to content

Fix for Potential memory leak in class ServiceArgumentParser's Constructor #8367

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
Closed
pradishmp wants to merge 0 commits into from
Closed

Fix for Potential memory leak in class ServiceArgumentParser's Constructor #8367

pradishmp wants to merge 0 commits into from

Conversation

@pradishmp
Copy link
Contributor

@pradishmp pradishmp commented Jul 3, 2024 •
edited by directionless
Loading

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 remove the call to the function clearArgs() and leave it to the destructor to clean up the memory.

@pradishmp pradishmp requested review from a team as code owners July 3, 2024 14:40
@zwass
Copy link
Member

zwass commented Jul 3, 2024

Reopen for CI fix

@zwass zwass closed this Jul 3, 2024
@zwass zwass reopened this Jul 3, 2024
directionless
directionless approved these changes Jul 3, 2024
View reviewed changes
Copy link
Member

@directionless directionless left a 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 okay -- there's a destructer that clears the args. @Smjert @zwass either of you?

@michael-myers
Copy link
Contributor

michael-myers commented Jul 3, 2024

I think this is okay -- there's a destructer that clears the args. @Smjert @zwass either of you?

Both before and after this PR, the same private method, cleanArgs will free the memory, only, it needs to be called only after owns_argv_ptrs_ = true;. Another way to fix this, maybe how I would do it, would be to set owns_argv_ptrs_ = true; immediately after the CommandLineToArgvW returns successfully. So, before the for loop instead of right after it. But this PR also fixes the issue.

@pradishmp pradishmp closed this Jul 4, 2024
@pradishmp
Copy link
Contributor Author

pradishmp commented Jul 4, 2024

i was trying to remove some of the older commits which are not related to this issue..ended up closing the PR, Apologies for the inconvenience caused. i can create a new PR and submit for review , please let me know your thoughts

@pradishmp
Copy link
Contributor Author

pradishmp commented Jul 4, 2024

I think this is okay -- there's a destructer that clears the args. @Smjert @zwass either of you?

Another way to fix this, maybe how I would do it, would be to set owns_argv_ptrs_ = true; immediately after the CommandLineToArgvW returns successfully. So, before the for loop instead of right after it.

i too agree with this approach, freeing memory immediately rather than wait for the destructor also make sence,

@Smjert
Copy link
Member

Smjert commented Jul 4, 2024

i was trying to remove some of the older commits which are not related to this issue..ended up closing the PR, Apologies for the inconvenience caused. i can create a new PR and submit for review , please let me know your thoughts

Feel free to open a new PR, I suggest not using the master branch on your repo to submit the PR from, but a separate PR specific branch. This way you can track osquery master branch, work on your bugfix branch and also sync it against master with a rebase (don't use merge or be aware that git pull by default merges if it cannot do a fast forward merge, which can be changed).

@directionless
Copy link
Member

directionless commented Jul 4, 2024

@pradishmp We are likely to cut a release in the next few days, I don't know if the timing will line up on your next PR, but there will always be another release.

@pradishmp
Copy link
Contributor Author

pradishmp commented Jul 4, 2024

@pradishmp We are likely to cut a release in the next few days, I don't know if the timing will line up on your next PR, but there will always be another release.

ok i will see if i can fix this today itself or we can always have this in next release, whichever works best for the product.

@directionless directionless mentioned this pull request Jul 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@directionless directionless directionless approved these changes

Assignees

No one assigned

Labels

memory leak

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Potential memory leak in class ServiceArgumentParser's Constructor

5 participants

@pradishmp @zwass @michael-myers @Smjert @directionless