Don't call realpath(3) from libmap rtld code.

[trasz]

Summary

Don't call realpath(3) from libmap rtld code. This shaves a few calls to fstatat(2) at binary startup; the difference looks like this:

--- przed       2017-10-14 13:55:49.983528000 +0100
+++ po  2017-10-14 14:10:39.134343000 +0100
@@ -1,15 +1,10 @@
 mmap(0x0,32768,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANON,-1,0x0) = 34366173184 (0x800623000)
 issetugid()                                     = 0 (0x0)
-fstatat(AT_FDCWD,"/etc",{ mode=drwxr-xr-x ,inode=1364352,size=2560,blksize=32768 },AT_SYMLINK_NOFOLLOW) = 0 (0x0)
-fstatat(AT_FDCWD,"/etc/libmap.conf",{ mode=-rw-r--r-- ,inode=1373288,size=102,blksize=32768 },AT_SYMLINK_NOFOLLOW) = 0 (0x0)
 openat(AT_FDCWD,"/etc/libmap.conf",O_RDONLY|O_CLOEXEC,00) = 3 (0x3)
 fstat(3,{ mode=-rw-r--r-- ,inode=1373288,size=102,blksize=32768 }) = 0 (0x0)
 mmap(0x0,102,PROT_READ,MAP_PRIVATE,3,0x0)       = 34366205952 (0x80062b000)
 close(3)                                        = 0 (0x0)
-fstatat(AT_FDCWD,"/usr",{ mode=drwxr-xr-x ,inode=561792,size=512,blksize=32768 },AT_SYMLINK_NOFOLLOW) = 0 (0x0)
-fstatat(AT_FDCWD,"/usr/local",{ mode=drwxr-xr-x ,inode=561800,size=512,blksize=32768 },AT_SYMLINK_NOFOLLOW) = 0 (0x0)
-fstatat(AT_FDCWD,"/usr/local/etc",{ mode=drwxr-xr-x ,inode=653279,size=1536,blksize=32768 },AT_SYMLINK_NOFOLLOW) = 0 (0x0)
-fstatat(AT_FDCWD,"/usr/local/etc/libmap.d",0x7fffffffcf50,AT_SYMLINK_NOFOLLOW) ERR#2 'No such file or directory'
+open("/usr/local/etc/libmap.d",O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC,0165) ERR#2 'No such file or directory'
 munmap(0x80062b000,102)                                 = 0 (0x0)
 openat(AT_FDCWD,"/var/run/ld-elf.so.hints",O_RDONLY|O_CLOEXEC,00) = 3 (0x3)
 read(3,"Ehnt\^A\0\0\0\M^@\0\0\0\M-2\0\0"...,128) = 128 (0x80)

Note for reviewers: the patch replaces realpath(3) with xstrdup(3) slightly later on. Most of the diff is due to variable renaming (s/rpath/path/;s/rpath/idir/).

[kib]

libexec/rtld-elf/libmap.c

105 ↗

(On Diff #34177)

This is clearly done to avoid duplicate files to be included into the parsed config. Look a the next TAILQ_FOREACH() statement.

Perhaps it could be implemented differently, by storing device, inode number in struct lmc and matching against the pair. Simply removing the functionality is not useful.

156 ↗

(On Diff #34177)

realpath() is called there for similar reasons, but perhaps avoiding duplication with actual config files would make this removal only cosmetic and causing longer startup by iterating over the dups.

[mjg]

I think the first iteration of the patch should avoid realpath if there is nothing in the target directory.

In the longer run, I think the entire mechanism has to be reimplemented. Even if per-binary support (without adding work for *everyone*) turns out to be problematic, in the worst case this should read a small binary database instead of parsing text files. This has a side effect of not having to worry about loops etc.

Reading of libmap.conf itself is also weirdly pessimized with mmap/munmap instead of just mere read, especially given the small size of the file.

The db would be created by a dedicated tool which would parse text files and which would have to be executed on demand by sysadmins/pkg/whatever.

[trasz]

Also check st_dev and st_ino, to make sure we _really_ ignore duplicates.

[trasz]

libexec/rtld-elf/libmap.c

105 ↗

(On Diff #34177)

Yeah; I assumed it's ok to assume duplicates would have actually different paths, but you might be right. I've added the dev/ino check. The path check is still there - when we can tell if the file is the same because it has the same path, there's no point in calling open/fstat.

[kib]

libexec/rtld-elf/libmap.c

165 ↗

(On Diff #34183)

And this instance of struct lmc is entered the queue with random garbage in dev/ino fields, right ?

[trasz]

Don't leave uninitialized fields, d'oh!

[trasz]

libexec/rtld-elf/libmap.c

165 ↗

(On Diff #34183)

Sorry, it's been hard week. I promise I won't commit anything today. :-)

[kib]

libexec/rtld-elf/libmap.c

168 ↗

(On Diff #34185)

fstat(dirfd(d)) is not too hard.

[trasz]

libexec/rtld-elf/libmap.c

168 ↗

(On Diff #34185)

Yeah, but it's yet another syscall, and in this case it wouldn't serve any purpose - even if the path somehow matched, if the st_dev doesn't match (and it won't match due to NODEV), the st_ino won't matter.