Summary: | sys-apps/openrc-0.17: bootmisc: dangerous code in clean_run | ||
---|---|---|---|
Product: | Gentoo Hosted Projects | Reporter: | Christopher Head <bugs> |
Component: | OpenRC | Assignee: | OpenRC Team <openrc> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | mva |
Priority: | Normal | ||
Version: | unspecified | ||
Hardware: | AMD64 | ||
OS: | Linux | ||
Whiteboard: | |||
Package list: | Runtime testing required: | --- |
Description
Christopher Head
2015-09-23 03:49:08 UTC
Why do maintainer using > rm -rf $dir instead of > rmdir $dir # assuming $dir is empty, or it anyway shouldn't delete it otherwise ? And yes, it should also use "&&" after umount // although, calling `rm` on *anything* on the live system during installation is unacceptable for me. There is postinst for such things. (In reply to Vadim A. Misbakh-Soloviov (mva) from comment #1) > // although, calling `rm` on *anything* on the live system during > installation is unacceptable for me. There is postinst for such things. This isn’t during installation. This is at every boot. (In reply to Christopher Head from comment #2) > This isn’t during installation. This is at every boot. So, then removing is kinda acceptable, but, anyway, IMHO, it should use: `mountpoint -q $dev || rm` but not just `rm` (and, IMHO, -f switch is unneeded in such scripts). // I'll try to ping oRC devs in IRC about the bug (In reply to Vadim A. Misbakh-Soloviov (mva) from comment #3) > (In reply to Christopher Head from comment #2) > > This isn’t during installation. This is at every boot. > > So, then removing is kinda acceptable, but, anyway, IMHO, it should use: > `mountpoint -q $dev || rm` but not just `rm` (and, IMHO, -f switch is > unneeded in such scripts). Better would be rmdir; rm doesn’t remove directories at all (even if empty) unless -r is provided. Also, I see no reason to bother calling mountpoint; either umount && rmdir or just umount and then rmdir, in the latter case knowing that rmdir is safe because if the umount failed, it can’t do anything. (In reply to Christopher Head from comment #4) > Better would be rmdir; rm doesn’t remove directories at all (even if empty) > unless -r is provided. Also, I see no reason to bother calling mountpoint; > either umount && rmdir or just umount and then rmdir, in the latter case > knowing that rmdir is safe because if the umount failed, it can’t do > anything. First, I thin the same, But then I remember that, there can be a cases, when, for some reason, mountpoint-directory can be unclean when [system] mounting on it, so it will be non-empty after unmount, so rmdir will not remove it. That is why rm -r can be needed. Next part, is that it `umount` is not obliged to always exit with 0 on success and with 1 on failure. While mountpoint -q *is* obliged (but have a bit opposite meaning). (In reply to Vadim A. Misbakh-Soloviov (mva) from comment #5) > First, I thin the same, But then I remember that, there can be a cases, > when, for some reason, mountpoint-directory can be unclean when [system] > mounting on it, so it will be non-empty after unmount, so rmdir will not > remove it. That’s impossible. The directory is created a couple of lines earlier with “mktemp -d” and nothing is put inside it. It is guaranteed to be empty if the umount is successful. The point of clean_run was to make sure that the directory, "/run" doesn't have any contents. That is why it bind mounts it to the location in /tmp then removes the contents of that location. "/run" is a mount point and there should be no contents in that directory. If I shouldn't care about that, I can remove this function completely. (In reply to William Hubbs from comment #7) > The point of clean_run was to make sure that the directory, "/run" doesn't > have any contents. That is why it bind mounts it to the location in /tmp > then removes the contents of that location. "/run" is a mount point and > there should be no contents in that directory. > > If I shouldn't care about that, I can remove this function completely. It’s not the “rm -rf $dir/run/*” that I was talking about. I understand that’s meant to clean the /run directory of the underlying disk. I don’t know whether or not that’s important to do; cleaning that directory doesn’t sound particularly crazy to me. The thing I’m pointing out is the “rm -rf $dir” two lines below, which is meant to just remove the temporary directory after the bind-mount is unmounted. That’s really dangerous if the umount fails. This is fixed in commit 1558ad2. |