Re: git: 36d67475f549 - main - xinstall: fix dounpriv logic, add tests

Reply: Dag-Erling_Smørgrav : "Re: git: 36d67475f549 - main - xinstall: fix dounpriv logic, add tests"
In reply to: Dag-Erling Smørgrav : "git: 36d67475f549 - main - xinstall: fix dounpriv logic, add tests"
Go to: [ bottom of page ] [ top of archives ] [ this month ]
From: Jessica Clarke <jrtc27_at_freebsd.org>
Date: Wed, 03 Aug 2022 20:59:15 UTC
On 3 Aug 2022, at 13:04, Dag-Erling Smørgrav <des@freebsd.org> wrote:
> 
> The branch main has been updated by des:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=36d67475f5497664d33c41c2f6745dcb30b0ec42
> 
> commit 36d67475f5497664d33c41c2f6745dcb30b0ec42
> Author:     Dag-Erling Smørgrav <des@FreeBSD.org>
> AuthorDate: 2022-08-03 18:59:28 +0000
> Commit:     Dag-Erling Smørgrav <des@FreeBSD.org>
> CommitDate: 2022-08-03 19:03:49 +0000
> 
>    xinstall: fix dounpriv logic, add tests

This is quite a poor commit message. What was wrong with it? Especially
when the diff is cluttered with reformatting. I cannot obviously see
any behavioural changes, just some changes from & to && that I don’t
believe technically matter, even if poor practice and not intended.

Jess

>    Sponsored by:   Klara, Inc.
>    MFC after:      1 week
> ---
> usr.bin/xinstall/tests/install_test.sh | 80 ++++++++++++++++++++++++++++++++++
> usr.bin/xinstall/xinstall.c            | 15 +++----
> 2 files changed, 87 insertions(+), 8 deletions(-)
> 
> diff --git a/usr.bin/xinstall/tests/install_test.sh b/usr.bin/xinstall/tests/install_test.sh
> index c723a6e0d280..e95f40fc19e7 100755
> --- a/usr.bin/xinstall/tests/install_test.sh
> +++ b/usr.bin/xinstall/tests/install_test.sh
> @@ -404,6 +404,84 @@ symbolic_link_relative_absolute_common_body() {
> 	fi
> }
> 
> +atf_test_case set_owner_group_mode
> +set_owner_group_mode_head() {
> +	atf_set "require.user" "root"
> +}
> +set_owner_group_mode_body() {
> +	local fu=65531 fg=65531
> +	local cu=65532 cg=65532
> +	local u="$(id -u)"
> +	local g="$(id -g)"
> +	local m=0755 cm=4444
> +	printf "test" >testf
> +	atf_check chown "$fu:$fg" testf
> +	atf_check chmod "$m" testf
> +
> +	atf_check install testf testc
> +	atf_check_equal "$u:$g:10$m" "$(stat -f"%u:%g:%p" testc)"
> +
> +	atf_check install -o "$cu" testf testc
> +	atf_check_equal "$cu:$g:10$m" "$(stat -f"%u:%g:%p" testc)"
> +
> +	atf_check install -g "$cg" testf testc
> +	atf_check_equal "$u:$cg:10$m" "$(stat -f"%u:%g:%p" testc)"
> +
> +	atf_check install -o "$cu" -g "$cg" testf testc
> +	atf_check_equal "$cu:$cg:10$m" "$(stat -f"%u:%g:%p" testc)"
> +
> +	atf_check install -m "$cm" testf testc
> +	atf_check_equal "$u:$g:10$cm" "$(stat -f"%u:%g:%p" testc)"
> +
> +	atf_check install -o "$cu" -m "$cm" testf testc
> +	atf_check_equal "$cu:$g:10$cm" "$(stat -f"%u:%g:%p" testc)"
> +
> +	atf_check install -g "$cg" -m "$cm" testf testc
> +	atf_check_equal "$u:$cg:10$cm" "$(stat -f"%u:%g:%p" testc)"
> +
> +	atf_check install -o "$cu" -g "$cg" -m "$cm" testf testc
> +	atf_check_equal "$cu:$cg:10$cm" "$(stat -f"%u:%g:%p" testc)"
> +}
> +
> +atf_test_case set_owner_group_mode_unpriv
> +set_owner_group_mode_unpriv_head() {
> +	atf_set "require.user" "root"
> +}
> +set_owner_group_mode_unpriv_body() {
> +	local fu=65531 fg=65531
> +	local cu=65532 cg=65532
> +	local u="$(id -u)"
> +	local g="$(id -g)"
> +	local m=0755 cm=4444 cM=0444
> +	printf "test" >testf
> +	atf_check chown "$fu:$fg" testf
> +	atf_check chmod "$m" testf
> +
> +	atf_check install -U testf testc
> +	atf_check_equal "$u:$g:10$m" "$(stat -f"%u:%g:%p" testc)"
> +
> +	atf_check install -U -o "$cu" testf testc
> +	atf_check_equal "$u:$g:10$m" "$(stat -f"%u:%g:%p" testc)"
> +
> +	atf_check install -U -g "$cg" testf testc
> +	atf_check_equal "$u:$g:10$m" "$(stat -f"%u:%g:%p" testc)"
> +
> +	atf_check install -U -o "$cu" -g "$cg" testf testc
> +	atf_check_equal "$u:$g:10$m" "$(stat -f"%u:%g:%p" testc)"
> +
> +	atf_check install -U -m "$cm" testf testc
> +	atf_check_equal "$u:$g:10$cM" "$(stat -f"%u:%g:%p" testc)"
> +
> +	atf_check install -U -o "$cu" -m "$cm" testf testc
> +	atf_check_equal "$u:$g:10$cM" "$(stat -f"%u:%g:%p" testc)"
> +
> +	atf_check install -U -g "$cg" -m "$cm" testf testc
> +	atf_check_equal "$u:$g:10$cM" "$(stat -f"%u:%g:%p" testc)"
> +
> +	atf_check install -U -o "$cu" -g "$cg" -m "$cm" testf testc
> +	atf_check_equal "$u:$g:10$cM" "$(stat -f"%u:%g:%p" testc)"
> +}
> +
> atf_init_test_cases() {
> 	atf_add_test_case copy_to_nonexistent
> 	atf_add_test_case copy_to_nonexistent_safe
> @@ -444,4 +522,6 @@ atf_init_test_cases() {
> 	atf_add_test_case symbolic_link_relative_absolute_source_and_dest2
> 	atf_add_test_case symbolic_link_relative_absolute_common
> 	atf_add_test_case mkdir_simple
> +	atf_add_test_case set_owner_group_mode
> +	atf_add_test_case set_owner_group_mode_unpriv
> }
> diff --git a/usr.bin/xinstall/xinstall.c b/usr.bin/xinstall/xinstall.c
> index 05b1444506db..ddad7ba9115e 100644
> --- a/usr.bin/xinstall/xinstall.c
> +++ b/usr.bin/xinstall/xinstall.c
> @@ -1009,19 +1009,18 @@ install(const char *from_name, const char *to_name, u_long fset, u_int flags)
> #endif
> 	}
> 
> -	if (!dounpriv & 
> -	    (gid != (gid_t)-1 && gid != to_sb.st_gid) ||
> -	    (uid != (uid_t)-1 && uid != to_sb.st_uid))
> +	if (!dounpriv && ((gid != (gid_t)-1 && gid != to_sb.st_gid) ||
> +	    (uid != (uid_t)-1 && uid != to_sb.st_uid))) {
> 		if (fchown(to_fd, uid, gid) == -1) {
> 			serrno = errno;
> 			(void)unlink(to_name);
> 			errno = serrno;
> 			err(EX_OSERR,"%s: chown/chgrp", to_name);
> 		}
> -
> +	}
> 	if (mode != (to_sb.st_mode & ALLPERMS)) {
> 		if (fchmod(to_fd,
> -		     dounpriv ? mode & (S_IRWXU|S_IRWXG|S_IRWXO) : mode)) {
> +		    dounpriv ? mode & (S_IRWXU|S_IRWXG|S_IRWXO) : mode)) {
> 			serrno = errno;
> 			(void)unlink(to_name);
> 			errno = serrno;
> @@ -1036,7 +1035,7 @@ install(const char *from_name, const char *to_name, u_long fset, u_int flags)
> 	 * trying to turn off UF_NODUMP.  If we're trying to set real flags,
> 	 * then warn if the fs doesn't support it, otherwise fail.
> 	 */
> -	if (!dounpriv & !devnull && (flags & SETFLAGS ||
> +	if (!dounpriv && !devnull && (flags & SETFLAGS ||
> 	    (from_sb.st_flags & ~UF_NODUMP) != to_sb.st_flags) &&
> 	    fchflags(to_fd,
> 	    flags & SETFLAGS ? fset : from_sb.st_flags & ~UF_NODUMP)) {
> @@ -1082,7 +1081,7 @@ compare(int from_fd, const char *from_name __unused, size_t from_len,
> 		return 1;
> 
> 	do_digest = (digesttype != DIGEST_NONE && dresp != NULL &&
> -			*dresp == NULL);
> +	    *dresp == NULL);
> 	if (from_len <= MAX_CMP_SIZE) {
> 		if (do_digest)
> 			digest_init(&ctx);
> @@ -1390,7 +1389,7 @@ install_dir(char *path)
> 			ch = *p;
> 			*p = '\0';
> again:
> -			if (stat(path, &sb) < 0) {
> +			if (stat(path, &sb) != 0) {
> 				if (errno != ENOENT || tried_mkdir)
> 					err(EX_OSERR, "stat %s", path);
> 				if (mkdir(path, 0755) < 0) {