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

Fix #311, Free (close) FileDescriptor resource to avoid leak #370

Closed

Conversation

thnkslprpt
Copy link
Contributor

Checklist*

Describe the contribution
Fixes #311
Added additional close() for FileDescriptor which was missing before the end of the function.

Testing performed
GitHub CI actions (incl. Build + Run, Unit Tests etc.) all passing successfully.

Expected behavior changes
FileDescriptor freed (closed) before going out of scope, avoiding slight resource leak.

Contributor Info
Avi @thnkslprpt

Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

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

I wasn't sure about this one, but based on this comment it should be OK: https://stackoverflow.com/questions/17490033/do-i-need-to-keep-a-file-open-after-calling-mmap-on-it

What Coverity doesn't know (and why this was reported as a "leak") is that this is a setup function that is only called once at start-up, and the file is supposed to remain available for the entire duration of the CFE instance, until the parent process exits - at which time the filehandle is closed automatically. So while not ideal, it was not an actual resource leak.

We should confirm, however, that the simulated EEPROM here still works after closing the file handle - that is, read/write of the mapped memory still works and updates the file on disk. The man page of mmap says its supposed to, but I would recommend confirming that it actually does.

@dzbaker dzbaker added this to the Fornax milestone Nov 21, 2022
@dzbaker dzbaker modified the milestones: Fornax, Equuleus Dec 7, 2022
@thnkslprpt
Copy link
Contributor Author

Closing this PR as per comments in #311 (comment)

@thnkslprpt thnkslprpt closed this Feb 23, 2023
@thnkslprpt thnkslprpt deleted the fix-311-free-FileDescriptor-resource branch February 23, 2023 22:38
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.

Resource Leak
3 participants