Quick'n'dirty fix for the 'failed to remove /tmp' bug

[AltGr]

No description provided.

[kit-ty-kate]

Note that more insidious issues could happen if someone would execute opam as root. What was the incentive to remove the parent directory in opam 2.1 that wasn't there in opam 2.0?

[avsm]

What's the cause of this "failed to remove /tmp" bug? Was it reported somewhere else?

[rjbou]

@kit-ty-kate It comes from the new repository mechanism that uses /tmp to uncompress repo archives. And on root issues, apart that opam tells you to not use it as root, this leads to suppress an empty directory, even if opam should'nt do it, there is no loss.
@avsm Not reported, it is a kind of container issue (@kit-ty-kate found it), as in these environment /tmp is sometimes empty. It is not usual on desktop environments.

[avsm]

Thanks for the explanation, @rjbou. I still don't understand what's going on -- why are we trying to delete /tmp ever, even if it doesn't exist?

[rjbou]

Sometimes, when removing temporary stuff, a directory cleaning is done, to remove all intermediate temporary directories. The process is upward: if directory is empty, removes it, and check its on parent, ... On some new containers, it reaches /tmp, as its only content was those opam temporary files.

[avsm]

I see, thanks for the explanation. That's a definite potential security issue, since those parent directories might have been created by some other tool with specific semantics (e.g. a sticky bit).

opam should only remove those directories that it has created, and no others.

[AltGr]

Note that more insidious issues could happen if someone would execute opam as root. What was the incentive to remove the parent directory in opam 2.1 that wasn't there in opam 2.0?

Nothing new in 2.1 here, this behaviour was in 1.2, and probably even before that.
A simple safeguard could be to never remove parent directories with depth < 2.
@avsm I agree that would be best, but can't think of a practical way to keep track of everything that opam has created. So the cleaner solution here would be to check all use-cases of this "cleanup" function (I don't think there are that many) and write their desired behaviour more explicitely.