search

eminence / procfs

Rust library for reading the Linux procfs filesystem
Other
366 stars 106 forks source link

[Bug] `FDsIter` memory leak #287

Closed cyqsimon closed 11 months ago

cyqsimon commented 12 months ago

TLDR

There seems to be a memory leak out of FDsIter. When used in a hot path, leaked memory quickly accumulates to gigabyte sizes and even triggers OOM crashes.

More details

This is the initial bug report: imsnif/bandwhich#284. Flamegraph and heaptrack data available there.

This is where the leak occurs out of bandwhich, according to heaptrack: https://github.com/imsnif/bandwhich/blob/4a27da5d5627c7eff0ac8c17c6c031db1879b6d1/src/os/linux.rs#L16.

It seems to be caused by rustix::fs::Dir::read_from here:

https://github.com/eminence/procfs/blob/e8e9bc101840378d42119076eec5b5a721a446b2/procfs/src/process/mod.rs#L478

We have also tested both gnu and musl builds and saw the same problem. So we are no longer suspecting an allocator fault at this point.

Reproducibility

It does not seem like this issue is reproducible on all Linux distros, but several users have reported this problem so it's unlikely to be bogus. So far we have the following datapoints:

Problem seen on Problem not seen on
Ubuntu 22.04 LTS EndeavourOS
Void Linux

I'll populate the table some more as information comes in.

Versions tested

cyqsimon commented 12 months ago

I haven't submitted an issue in rustix yet because it's not our direct dependency, and I would like to hear what procfs maintainers would like to say first. However I'm happy to do so if requested.

cyqsimon commented 12 months ago

@konnorandrews was able to track down this issue to a problem in rustix in https://github.com/imsnif/bandwhich/issues/284#issuecomment-1754321993. I have reported it as a vulnerability in rustix.

I will keep this issue open, but there is no action required from procfs for now.

eminence commented 12 months ago

Nice investigative work @konnorandrews and @cyqsimon. If you have it, can you link the rustix issue? It will be useful to following the evolution of the issue, and to know if procfs needs to bump a dependency to get the fix. Thanks!

cyqsimon commented 12 months ago

https://github.com/bytecodealliance/rustix/security/advisories/GHSA-c827-hfw6-qwvm

It was posted as a security advisory so it will be confidential until it's patched. If you want though I can request rustix's maintainers to give you access.

eminence commented 12 months ago

Thanks for the link. No need for me to get early access, I can find out the details when the rest of the world does :)

sigmaSd commented 12 months ago

I can't reproduce on archlinux, @cyqsimon did you try it on one of the affected distro?, this all would be easier if one could reproduce the issue moved to https://github.com/imsnif/bandwhich/issues/284 to try to keep the conversation mostly in one place

cyqsimon commented 11 months ago

Hello again, the fix has been released and backported to all affected minor versions of rustix (0.35.15, 0.36.16, 0.37.25, 0.38.19).

Technically I don't think there is any action required from procfs, since procfs doesn't pin to a particular version of rustix, but you may wish to bump it nonetheless. Please feel free to deal with this issue any way you see fit.

eminence commented 11 months ago

Thank you everyone, for the initial report, for the investigation, and for the fix. This was an excellent demonstration of the value and the power of Open Source. The minimum rustix version has been bumped to 0.38.19