bin/124409: fsck(8) requires exact entry for mountpoints when executing / bug by design in getfsfile(3) in .../lib/libc/gen/fstab.c

[Garrett Cooper]

On Sat, Jun 21, 2008 at 8:10 PM, Giorgos Keramidas <keramida at freebsd.org> wrote:
> The following reply was made to PR bin/124409; it has been noted by GNATS.
>
> From: Giorgos Keramidas <keramida at freebsd.org>
> To: "Garrett Cooper" <yanefbsd at gmail.com>
> Cc: bug-followup at freebsd.org
> Subject: Re: bin/124409: fsck(8) requires exact entry for mountpoints when executing / bug by design in getfsfile(3) in .../lib/libc/gen/fstab.c
> Date: Sun, 22 Jun 2008 06:07:37 +0300
>
>  On Mon, 16 Jun 2008 20:28:08 -0700, "Garrett Cooper" <yanefbsd at gmail.com> wrote:
>  > On Tue, Jun 10, 2008 at 2:27 AM, Garrett Cooper <yanefbsd at gmail.com> wrote:
>  >> Ok.... it appears I wasn't intelligent enough to post this in the right
>  >> place last night. Comments please?
>  >>
>  >> Hi hackers,
>  >>
>  >>      I have a question, pending a bug found in getfsfile(3) [1].
>  >>
>  >>      Is there any possibility where a mountpoint be any value other
>  >>
>  >> than a directory, a symlink, or "none", i.e. a flat file?
>  >>
>  >> Thanks,
>  >>
>  >> -Garrett
>  >>
>  >> References:
>  >>
>  >> http://www.freebsd.org/cgi/query-pr.cgi?pr=bin/124409 (not fully in PR
>  >>
>  >> database yet).
>  >
>  > Going once, going twice...
>
>  Hi Garrett :-)
>
>  When I wrote the original comment in that PR I had something like this
>  in mind:
>
>  %%%
>  *** src.7789d2851415/lib/libc/gen/fstab.c      Sun Jun 22 05:57:09 2008
>  --- /home/build/src/lib/libc/gen/fstab.c       Sun Jun 22 05:56:48 2008
>  *************** struct fstab *
>  *** 236,245 ****
>   getfsfile(name)
>        const char *name;
>   {
>        if (setfsent())
>  !              while (fstabscan())
>  !                      if (!strcmp(_fs_fstab.fs_file, name))
>                                return(&_fs_fstab);
>        return((struct fstab *)NULL);
>   }
>
>  --- 236,256 ----
>   getfsfile(name)
>        const char *name;
>   {
>  +      char name_path[PATH_MAX];
>  +      char fstab_path[PATH_MAX];
>  +
>  +      if (realpath(name, name_path) == NULL)
>  +              return((struct fstab *)NULL);
>        if (setfsent())
>  !              while (fstabscan()) {
>  !                      if (strcmp(_fs_fstab.fs_file, name) == 0 ||
>  !                          strcmp(_fs_fstab.fs_file, name_path) == 0)
>  !                              return(&_fs_fstab);
>  !                      if (realpath(_fs_fstab.fs_file, fstab_path) == NULL)
>  !                              return((struct fstab *)NULL);
>  !                      if (strcmp(fstab_path, name_path) == 0)
>                                return(&_fs_fstab);
>  +              }
>        return((struct fstab *)NULL);
>   }
>
>  %%%
>
>  I tried to avoid unnecessary realpath(3) calls as much as possible, but
>  there is still the possibility for at least N+1 calls, where N is the
>  number of entries in fstab...
>
>  Can you test the above patch, and let me know if it looks ok, if you
>  have a better fix in the works, etc.?  It seems to pass the bug you
>  originally reported when I run:
>
>     % env LD_PRELOAD=/home/build/obj/home/build/src/lib/libc/libc.so.7 \
>         fsck ///cdrom///
>     fsck: exec fsck_cd9660 for /dev/acd0 in /sbin:/usr/sbin: No such file or directory
>
>  The failure later is ok, because we don't have fsck_cd9660 for CD-ROMs.

There was a reason why I avoided going straight to realpath... didn't
realpath have security issues and the like, such that it wasn't

Linux (it isn't included in glibc) uses a function called
canonicalize() for resolving paths, which does something similar to
realpath, but doesn't resolve symlinks, IIRC, and thus avoids the
security / race condition issue.

Cheers,
-Garrett