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 memory leak when evicting or expiring an entry (v0.7.x) #65

Merged
merged 6 commits into from
Jan 11, 2022

Conversation

tatsuya6502
Copy link
Member

@tatsuya6502 tatsuya6502 commented Jan 11, 2022

This PR addresses the memory leak issue reported by #64.

The memory leak happens when evicting/expiring an entry or manually invalidating an entry. It should not happen when dropping a cache.

The leaking data is the key of an entry, last modified timestamp (u64) and last accessed timestamp (u64). It does not include the value of an entry.

How to reproduce

  • This issue can be reproduced by the code in the description of Memory leak #64.
  • Or by running the following commands. Miri will detect similar issue in Deque's unit tests:
$ rustup update nightly
$ rustup +nightly component add miri
$ cargo +nightly miri test deque
running 4 tests
test common::deque::tests::basics ... ok
test common::deque::tests::drop ... ok
test common::deque::tests::iter ... ok
test common::deque::tests::next_node ... ok

test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 39 filtered out

The following memory was leaked: alloc92503 (Rust heap, size: 1, align: 1) {
    61                                              │ a
}
alloc92586 (Rust heap, size: 48, align: 8) {
    0x00 │ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 │ ................
    0x10 │ ╾0x23b1f7[a92503]<untagged> (8 ptr bytes)╼ 01 00 00 00 00 00 00 00 │ ╾──────╼........
    0x20 │ 01 00 00 00 00 00 00 00 01 __ __ __ __ __ __ __ │ .........░░░░░░░
}
alloc93007 (Rust heap, size: 1, align: 1) { ... }
alloc93087 (Rust heap, size: 48, align: 8) { ... }
alloc93620 (Rust heap, size: 1, align: 1) { ... }
alloc93700 (Rust heap, size: 48, align: 8) { ... }
alloc105252 (Rust heap, size: 1, align: 1) { ... }
alloc105323 (Rust heap, size: 48, align: 8) { ... }
alloc114558 (Rust heap, size: 1, align: 1) { ... }
alloc114629 (Rust heap, size: 48, align: 8) { ... }

error: the evaluated program leaked memory

Cause

The root cause is forgetting to drop an unlinked DeqNode after calling an unsafe method moka::common::Deque::unlink.

Fixes

  • Added unsafe unlink_and_drop method to Deque and updated the callers of unlink method to call unlink_and_drop. As the name implies, unlink_and _drop will unlink the DeqNode and then drop it.
  • Added Miri test to the CI to detect similar programming errors in future.

This PR is for the master branch (v0.7.x). There is another PR #66 with the same fixes for the maint-06 branch (v0.6.x).

Affected Versions

  • v0.1.0 to v0.5.4
  • v0.6.0 to v0.6.3. (Fixed in v0.6.4)
  • v0.7.0. (Fixed in v0.7.1)

Notes

We are not yanking the affected versions at crates.io although this issue will have impact to almost all Moka users, and may lead out of memory error in some use cases. This is because memory leak is not considered a memory safety issue and does not lead undefined behavior (UB).

Instead, we are doing patch releases v0.6.4 and v0.7.1 with the fixes, so that existing v0.6 and v0.7 users will automatically move to one of these fixed versions when cargo update is executed.


Fixes #64.

@tatsuya6502 tatsuya6502 self-assigned this Jan 11, 2022
@tatsuya6502 tatsuya6502 added the bug Something isn't working label Jan 11, 2022
@tatsuya6502 tatsuya6502 added this to the v0.7.1 milestone Jan 11, 2022
…an entry

Add `unlink_and_drop` method to `common::Deque`.
…an entry

Add GitHub CI to run Miri test on the `deque` module.
…an entry

Add GitHub CI to run Miri test on the `deque` module.
…an entry

Add GitHub CI to run Miri test on the `deque` module.
@tatsuya6502 tatsuya6502 changed the title Fix memory leak when unlinking a DeqNode for evicting/expiring an entry Fix memory leak when evicting or expiring an entry (v0.7.x) Jan 11, 2022
tatsuya6502 added a commit that referenced this pull request Jan 11, 2022
Apply the same fix to #65 to Moka v0.6.x.
@tatsuya6502 tatsuya6502 marked this pull request as ready for review January 11, 2022 11:00
Copy link
Member Author

@tatsuya6502 tatsuya6502 left a comment

Choose a reason for hiding this comment

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

Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak
1 participant