Skip to content

Coverity Fix: Close opened mmd libraries #290

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

Merged
merged 2 commits into from
Jul 10, 2023
Merged

Conversation

tylerzhao7684
Copy link
Contributor

@tylerzhao7684 tylerzhao7684 commented May 1, 2023

Type: Resource leak (RESOURCE_LEAK)
mmd_library is not properly closed under several exit conditions, we need to close it before exiting

@tylerzhao7684
Copy link
Contributor Author

Seems like the mmd library is accessed globally, need another way to fix this

@tylerzhao7684 tylerzhao7684 requested a review from zibaiwan May 3, 2023 14:31
@tylerzhao7684
Copy link
Contributor Author

Confirmed that it is this line that causes the unit test to fail
@zibaiwan Any idea on this?
Coverity doesn't like mmd_library (A pointer) to be lost even though the library is accessed globally

@tylerzhao7684 tylerzhao7684 changed the title Coverity Fix: Free allocated memory before pointer is lost Coverity Fix: Close opened mmd libraries May 4, 2023
@tylerzhao7684
Copy link
Contributor Author

tylerzhao7684 commented May 4, 2023

I looked at #227 and used a similar method to close the library
Also looked at #264 and I think it caused a problem because in some cases dlsym need to be called on libraries that are opened globally

@tylerzhao7684
Copy link
Contributor Author

@zibaiwan Integration test passed with this implementation
Review please!

zibaiwan
zibaiwan previously approved these changes May 30, 2023
@zibaiwan
Copy link
Contributor

@tylerzhao7684 , as we are very close to merge this in. Assuming the window testing passes, could you please squash your 4 commits into 1 commit as they essentially all for fixing one issue?

The configuration in this repo is set to merge all 4 commits into the repo instead of squashing them into 1 automatically, to allow one large PR can have multiple meaningful commits. You can choose the way you like to do the squash, e.g., rebease -i or reset --soft, and then do a force push. Let me know if you have any questions!

Thanks you!

@zibaiwan
Copy link
Contributor

@tylerzhao7684 , can you please take a look at my previous comment to squash the change? This PR is ready to merge once the commit is cleaned up.

Using destructor to close MMD libraries

Clang format

Closing mmd_lib in destructor that is not opened globally
@zibaiwan
Copy link
Contributor

@tylerzhao7684 , can you please take a look at the workflow failure? My guess is, cov-errors.txt is now empty based on the empty output of the cat cov-errors.txt because you fixed the last Coverity issue. However, I don't understand why count=$(grep -c '^$' cov-errors.txt) && echo "$(( $count / 2 ))" would fail in this case, instead of just outputting 0.

One possible guess is: cov-errors.txt didn't exist and the grep failed, but it didn't make sense why cat cov-errors.txt didn't fail earlier. Can you please investigate a bit and this PR likely needs a supplement change in the workflow as well, otherwise the main branch workflow will show the failure status.

@tylerzhao7684
Copy link
Contributor Author

This previous workflow didn't fail because #277 was not merged in, so even if this PR fixes one coverity issue, the other one still remain as it is not merged
Here's the previous workflow run

Now it fails because it couldn't find cov-errors.txt, which is a good sign because now we don't have coverity issues anymore!
I'll work on a small fix in this PR to make this workflow succeed when cov-errors.txt is not found

@zibaiwan
Copy link
Contributor

This previous workflow didn't fail because #277 was not merged in, so even if this PR fixes one coverity issue, the other one still remain as it is not merged Here's the previous workflow run

Now it fails because it couldn't find cov-errors.txt, which is a good sign because now we don't have coverity issues anymore! I'll work on a small fix in this PR to make this workflow succeed when cov-errors.txt is not found

I am not 100% sure if it indeed failed due to cov-errors.txt is not found, because in the workflow, cat cov-errors.txt step didn't fail. I tested it locally on one of the linux machine, this would give an error like this cat: dummy.txt: No such file or directory, but maybe the workflow machine has a different linux distribution therefore no error? But if it indeed caused by no file found, I 100% agree this is a good sign!

@zibaiwan
Copy link
Contributor

This previous workflow didn't fail because #277 was not merged in, so even if this PR fixes one coverity issue, the other one still remain as it is not merged Here's the previous workflow run

Now it fails because it couldn't find cov-errors.txt, which is a good sign because now we don't have coverity issues anymore! I'll work on a small fix in this PR to make this workflow succeed when cov-errors.txt is not found

A separate commit for the workflow fix is more appropriate (no need to squash) but can be merged in the same PR.

@tylerzhao7684
Copy link
Contributor Author

This previous workflow didn't fail because #277 was not merged in, so even if this PR fixes one coverity issue, the other one still remain as it is not merged Here's the previous workflow run
Now it fails because it couldn't find cov-errors.txt, which is a good sign because now we don't have coverity issues anymore! I'll work on a small fix in this PR to make this workflow succeed when cov-errors.txt is not found

I am not 100% sure if it indeed failed due to cov-errors.txt is not found, because in the workflow, cat cov-errors.txt step didn't fail. I tested it locally on one of the linux machine, this would give an error like this cat: dummy.txt: No such file or directory, but maybe the workflow machine has a different linux distribution therefore no error? But if it indeed caused by no file found, I 100% agree this is a good sign!

You are correct, there is a cov-errors.txt.
The workflow failed because grep returns non-zero exit code when it finds no match, I added the fix now

@zibaiwan zibaiwan merged commit afad3c4 into intel:main Jul 10, 2023
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.

2 participants