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

Tighten bound on mlist isopen asserts #921

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

geky
Copy link
Member

@geky geky commented Jan 16, 2024

Unbalanced open/close calls continue to be a pain point for users, it doesn't help that this sometimes results in hard-to-debug infinite loops caused by the open-file linked-list (the mlist) getting tangled up in itself.

Moving the mlist isopen asserts lower into the actual list append/remove functions will help:

  1. Make sure coverage of potential linked-list issues is complete.

  2. Also assert against multiple close calls, which isn't an issue for the mlist, but can result in double free and memory corruption.

Unbalanced open/close calls continue to be a pain point for users, it
doesn't help that this sometimes results in hard-to-debug infinite loops
caused by the open-file linked-list (the mlist) getting tangled up in
itself.

Moving the mlist isopen asserts lower into the actual list append/remove
functions will help:

1. Make sure coverage of potential linked-list issues is complete.

2. Also assert against multiple close calls, which isn't an issue for
   the mlist, but can result in double free and memory corruption.
@geky geky added enhancement needs minor version new functionality only allowed in minor versions labels Jan 16, 2024
@geky geky mentioned this pull request Jan 16, 2024
@geky geky added next minor and removed needs minor version new functionality only allowed in minor versions labels Jan 17, 2024
@geky geky added this to the v2.9 milestone Jan 17, 2024
@geky
Copy link
Member Author

geky commented Jan 17, 2024

It looks like this doesn't work because of the restriction of LFS_ASSERT to error-returning functions (to allow asserts to return errors).

Tabling this for now. Current thinking is we should eventually split LFS_ASSERT into LFS_ASSERT and LFS_DISK_ASSERT, with only LFS_DISK_ASSERT being limited to error-returning functions.

@geky geky added needs minor version new functionality only allowed in minor versions postponed and removed next minor labels Jan 17, 2024
@geky geky marked this pull request as draft January 17, 2024 21:42
@geky geky removed this from the v2.9 milestone Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs minor version new functionality only allowed in minor versions postponed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant