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

Leaking tmp files in currect release (fixed in master) #83

Open
Cocalus opened this issue Jan 18, 2020 · 2 comments
Open

Leaking tmp files in currect release (fixed in master) #83

Cocalus opened this issue Jan 18, 2020 · 2 comments

Comments

@Cocalus
Copy link

Cocalus commented Jan 18, 2020

This is extracting from my incorrectly diagnosed #82

The issue is fixed in master by commit 15eb11c. So it does still affect the lastest release.

To test it requires the mkstemp fallback being used, which needs either Freebsd or an Linux kernel older than 3.17 (when memfd_create was added). I'm using 3.13. Before the fix in master files the tmp files from mkstemp would only be deleted on reboot, so every slice_deque would eat /tmp/ (and thus memory) even after the program closed.

I miss diagnosed the cause in #82. Personally I'd prefer an explicit clone over just a dereference, since it is more obvious (if the * was missing it would have been an issue, but rustc may catch that one). But that is a style choice. The changes from *mut to *const in #82 are also more explicit and arguably better, but don't affect correctness either.

Based on reading the memfd_create(2) the name could be any cstring, since it can collide, and the name is only externally visible from /proc/. The pattern is needed for mkstemp. I would suggest using the pattern just for mkstemp and some a plane &'static str like "slice_deque" for memfd_create, to make the libc/syscall API differences explicit in the code.

I can do another PR with my code clean ups if you want. They would have stopped me jumping to conclusions at least.

Once the fix from master is in a release I'll try to PR germangb/minimp3-rs which is my direct dependency and is using 0.2.4.

@gnzlbg
Copy link
Owner

gnzlbg commented Jan 19, 2020

Personally I'd prefer an explicit clone over just a dereference, since it is more obvious (if the * was missing it would have been an issue, but rustc may catch that one). But that is a style choice.

Here I would prefer to just use a type annotation to make things clearer (let mut fname: [u8; _] = *b"...";) since I agree with you that right now one has to think too much to follow that part of the code - or at least, I had to do that when reviewing your PR and I'm the one who wrote that code in the first place 😆

The changes from *mut to *const in #82 are also more explicit and arguably better, but don't affect correctness either.

I agree, we should make this change before the next release.


I think I will make these changes as part of #80 and try to merge that as soon as possible and do a new release. I hope I have time to devote to this tonight.

@Cocalus
Copy link
Author

Cocalus commented Jan 20, 2020

Thanks

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

No branches or pull requests

2 participants