Allow mergemaster to ignore type mismatches for specified files

[code.jpe_gmail.com]

Summary

This patch adds support for the new option ALLOW_SYMLINK which can be used to specify which files are allowed to have a file type mismatch if the mismatch is caused by a symbolic link to the correct type. If the installation target is changed via DESTDIR, the symlink is not allowed to escape from it.

ALLOW_SYMLINK is hardcoded to always allow /boot to mismatch, since a bsdinstall configurations exist that create /boot as symlink to an unencrypted boot pool. In this case ignoring the mismatch during mergemaster causes mergemaster to also ignore - and thus not update - any files below /boot.

It is hardcoded to avoid introducing a mergemaster configuration file newly into the base install.

[bcr]

You should add a few more src people to the review than just the manpages group, because there are also some changes in the shell script.

[code.jpe_gmail.com]

manpages group was automatically added. Since mergemaster has no entry in MAINTAINERS, I've added the last two that touched mergemaster.

[jilles]

usr.sbin/mergemaster/mergemaster.8

394

In what cases is IGNORE_TYPE_FILES useful?

398

ALLOW_SYMLINK seems to make sense, although some of these may be configured correctly by default instead of requiring customization (but requiring customization is the traditional mergemaster way of working).

usr.sbin/mergemaster/mergemaster.sh

1053

stat's -L flag can be used instead of a realpath invocation.

1054

Although this issue already exists in the surrounding code, ${SYMLINK_TARGET} (${DESTDIR}${COMPFILE#.} with the above comment), [:upper:] and [:lower:] should be quoted to avoid pathname generation and word splitting.

1057

This test checks that a string is non-empty which is definitely not empty. This is not what you seem to mean.

Also, prefer a case statement or two tests with || (you will need braces) to one test with -o.

1060

Consider

case " $ALLOW_SYMLINK " in
    *" ${COMPFILE#.} "*) continue ;;
esac

The same thing for the below loop.

[code.jpe_gmail.com]

usr.sbin/mergemaster/mergemaster.8

394

Honestly, mostly for providing sysadmins a way to shoot themselves into the foot.

Replacing a symlink to a directory with a directory is the other safe mismatch I can easily think of. I added this as an available catchall instead of trying to model all possible mismatches in case the admin does weird things and really wants it. No objection to removing it.

398

The only type mismatch caused by a FreeBSD installation without user intervention that I am aware of is installing via bsdinstall with ZFS and enabled disk encryption while using GPT (or MBR before geliboot). This creates a second unencrypted bootpool and /boot becomes a symlink into that bootpool. It might be similar for UFS.
Ignoring the type mismatch in mergemaster via i calls rm -rf on the new files that mergemaster compares against, thus removing all files below /boot from the compare list leaving them unupdated.

Since this is conditional on specific bsdinstall selections, I would follow up this patch with an update to bsdinstall that creates /etc/mergemaster.rc when that selection is chosen during installation, instead of providing a default mergemaster.rc that would then also have to be handled by mergemaster to avoid overwriting a user created one.

usr.sbin/mergemaster/mergemaster.sh

1053

I can use /usr/bin/stat -f "%R" "${DESTDIR}${COMPFILE#.}" instead of /bin/realpath "${DESTDIR}${COMPFILE#.}", no problem. But since the manpage says the data for these field specifiers does not come directly from struct stat, my guess it that it calls realpath in the background. I also considered it more readable than a stat format string.

Neither stat -L -f %N or stat -L -f %Y give me the absolute path pointed at by a relative symlink, which is required later to check that the symlink does not escape a changed basepath. What am I missing?

1057

$DESTDIR is only set if mergemaster is started with -D /different/basepath, mergemaster uses for example ${DESTDIR}/ or ${DESTDIR}/var/db to refer to / or /var/db if called without a changed base directory.

This line checks if ${DESTDIR} is unset and the user has therefor not changed the basepath, or when ${DESTDIR} is set, then ${SYMLINK_TARGET} must change if ${DESTDIR} prefix is removed. If ${SYMLINK_TARGET} has no prefix that matches ${DESTDIR} and stays the same, then the symlink points outside of the argument provided basepath.

[jilles]

usr.sbin/mergemaster/mergemaster.8

398

Providing a default mergemaster.rc is indeed inconvenient. However, perhaps we can hard-code /boot as allowed to be a symlink to a directory.

usr.sbin/mergemaster/mergemaster.sh

1053

Oh right, you do need ${SYMLINK_TARGET} below. Ignore my comment :)

1057

That's not what it actually does because the quoting is wrong ;)

[code.jpe_gmail.com]

usr.sbin/mergemaster/mergemaster.8

398

Good point. Adding it hard coded has the benefit of also transparently handling it for existing installations while being safe on installations that do not have the mismatch.

[code.jpe_gmail.com]

• removed IGNORE_TYPE_FILES
• updated manpage comment describing ALLOW_SYMLINK
• handled previously missed case of a relative destination directory, mergemaster -D ../foo
• improved variable quoting
• replaced loop matching with case statement as requested

[jilles]

Looks good except for the missing DESTDIR error check.

usr.sbin/mergemaster/mergemaster.sh

359

The quoting is correct here, but there should be an error check. Otherwise, a non-existent DESTDIR will cause mergemaster to install to /.

[code.jpe_gmail.com]

Added missing error check.

[jilles]

Sorry for the delay.

[des]

IMO mergemaster(8) should be deprecated, if not outright removed. We have had etcupdate(8) for years now. It does everything that mergemaster(8) does, except better.

[code.jpe_gmail.com]

I've updated the differential description to make it more clear what this patch does, as well as its intention regarding /boot.