-Wextra: with extra cheese #801

alejandro-colomar · 2023-09-02T16:11:24Z

Add -Wextra to have extra warnings.

alejandro-colomar · 2023-10-20T13:53:15Z

v2 changes:

Add libbsd CFLAGS and LIBS to the STRLCPY() test.

Tested locally:

$ sudo docker build -f ./share/containers/debian.dockerfile . | ovr
Successfully built a7538f4af26b

Range diff:

$ git range-diff asprintf gh/wextra wextra 
 1:  37a05f20 !  1:  a0b2b330 tests/unit/test_strlcpy.c: Add tests for strlcpy_()
    @@ tests/unit/Makefile.am: test_logind_LDADD = \
     +    $(NULL)
     +test_strlcpy_CFLAGS = \
     +    $(AM_FLAGS) \
    ++    $(LIBBSD_CFLAGS) \
     +    $(NULL)
     +test_strlcpy_LDFLAGS = \
     +    $(NULL)
     +test_strlcpy_LDADD = \
     +    $(CMOCKA_LIBS) \
    ++    $(LIBBSD_LIBS) \
     +    $(NULL)
     +
      test_xasprintf_SOURCES = \
 2:  e5fb15ce =  2:  18e8c542 lib/strlcpy.[ch]: Fix return type
 3:  05734c91 =  3:  088933c2 autogen.sh: CFLAGS: Add -Wextra
 4:  69bffef5 =  4:  a53a6a09 lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning
 5:  1482056e =  5:  26902c49 lib/commonio.c: Use uintmax_t to print nlink_t
 6:  02df1d98 =  6:  a5609d50 lib/strlcpy.h: Don't use a ternary op, to silence a -Wsign-compare warning
 7:  7ac7244d =  7:  873a4fc1 lib/copydir.c: Cosmetic
 8:  cb74fcf6 =  8:  6130019d lib/limits.c: Fix wrong error check
 9:  5da8f245 =  9:  ce059adc lib/limits.c: Check for overflow without invoking UB
10:  1f9ef7c3 = 10:  81bb33fa lib/loginprompt.c: Remove dead code
11:  7e54209b = 11:  9897030c lib/obscure.c: Mark parameter as [[maybe_unused]]
12:  fc0e231e = 12:  3bf5939c autogen.sh: CFLAGS: Add -Wno-expansion-to-defined
13:  aba59111 = 13:  4d2c09ef src/login_nopam.c: Add missing 'const'
14:  84935482 = 14:  6cfb89b5 src/gpasswd.c: Move if out of cpp conditional
15:  8bcd3abc = 15:  b1ad384d src/gpasswd.c: Mark failure() as [[noreturn]]
16:  f8300c70 = 16:  f4d27c90 src/gpasswd.c: Simplify cpp conditional
17:  6f1eca3f = 17:  5f5913da src/passwd.c: Reduce scope of cpp conditional
18:  69dbe8ed = 18:  c3e4de0b src/passwd.c: Mark parameters as [[maybe_unused]]

alejandro-colomar · 2023-10-20T17:55:15Z

v2b changes:

Reword commit message

$ git range-diff asprintf gh/wextra wextra
 1:  a0b2b330 !  1:  684514a3 tests/unit/test_strlcpy.c: Add tests for strlcpy_()
    @@ Metadata
     Author: Alejandro Colomar <[email protected]>
     
      ## Commit message ##
    -    tests/unit/test_strlcpy.c: Add tests for strlcpy_()
    +    tests/unit/test_strlcpy.c: Test strlcpy_() and STRLCPY()
     
    -    This test fails now, due to a bug: the return type is size_t, but it
    -    should be ssize_t.  The next commit will pass the test, by fixing the
    -    bug.
    +    This test fails now, due to a bug: the return type of strlcpy_() is
    +    size_t, but it should be ssize_t.  The next commit will pass the test,
    +    by fixing the bug.
     
         Signed-off-by: Alejandro Colomar <[email protected]>
     
 2:  18e8c542 =  2:  38e7cba0 lib/strlcpy.[ch]: Fix return type
 3:  088933c2 =  3:  eb365e47 autogen.sh: CFLAGS: Add -Wextra
 4:  a53a6a09 =  4:  d8e24f64 lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning
 5:  26902c49 =  5:  13798d2c lib/commonio.c: Use uintmax_t to print nlink_t
 6:  a5609d50 =  6:  0ee614c1 lib/strlcpy.h: Don't use a ternary op, to silence a -Wsign-compare warning
 7:  873a4fc1 =  7:  526aa8ea lib/copydir.c: Cosmetic
 8:  6130019d =  8:  a2dcbbd5 lib/limits.c: Fix wrong error check
 9:  ce059adc =  9:  98813fb1 lib/limits.c: Check for overflow without invoking UB
10:  81bb33fa = 10:  0c1cfaa4 lib/loginprompt.c: Remove dead code
11:  9897030c = 11:  18b64250 lib/obscure.c: Mark parameter as [[maybe_unused]]
12:  3bf5939c = 12:  e428fd7f autogen.sh: CFLAGS: Add -Wno-expansion-to-defined
13:  4d2c09ef = 13:  f3cd4318 src/login_nopam.c: Add missing 'const'
14:  6cfb89b5 = 14:  f46b72cc src/gpasswd.c: Move if out of cpp conditional
15:  b1ad384d = 15:  438274c5 src/gpasswd.c: Mark failure() as [[noreturn]]
16:  f4d27c90 = 16:  19764e85 src/gpasswd.c: Simplify cpp conditional
17:  5f5913da = 17:  99162724 src/passwd.c: Reduce scope of cpp conditional
18:  c3e4de0b = 18:  19c9bae5 src/passwd.c: Mark parameters as [[maybe_unused]]

alejandro-colomar · 2023-10-20T19:10:37Z

v2c changes:

Rebase to master

$ git range-diff alx/master..gh/wextra  master..wextra 
 1:  0f000ffe <  -:  -------- lib/copydir.c: Invert conditional to reduce nesting
 2:  4ed99325 <  -:  -------- lib/sprintf.[ch]: Add x[v]asprintf()
 3:  218c2dd3 <  -:  -------- lib/copydir.c: Use goto to reduce a conditional branch
 4:  8b765bd1 <  -:  -------- lib/, src/: Use asprintf(3) instead of strlen(3)+malloc(3)+snprintf(3)
 5:  2397f2fe <  -:  -------- lib/, src/: Use xasprintf() instead of its pattern
 6:  877c34ed <  -:  -------- autogen.sh: CFLAGS: Add -Werror=implicit-function-declaration
 7:  45171634 <  -:  -------- lib/: Add missing #include <config.h>
 8:  323c76bc <  -:  -------- autogen.sh: Prepare CFLAGS before ./configure
 9:  108f3e9e <  -:  -------- tests/unit/test_xasprintf.c: Test xasprintf()
10:  684514a3 =  1:  180160a0 tests/unit/test_strlcpy.c: Test strlcpy_() and STRLCPY()
11:  38e7cba0 =  2:  867f609b lib/strlcpy.[ch]: Fix return type
12:  eb365e47 =  3:  942d7f5c autogen.sh: CFLAGS: Add -Wextra
13:  d8e24f64 =  4:  effb8500 lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning
14:  13798d2c =  5:  3e836bcf lib/commonio.c: Use uintmax_t to print nlink_t
15:  0ee614c1 =  6:  349934a0 lib/strlcpy.h: Don't use a ternary op, to silence a -Wsign-compare warning
16:  526aa8ea =  7:  a668dfdd lib/copydir.c: Cosmetic
17:  a2dcbbd5 =  8:  8453864f lib/limits.c: Fix wrong error check
18:  98813fb1 =  9:  afa2200b lib/limits.c: Check for overflow without invoking UB
19:  0c1cfaa4 = 10:  6046bc95 lib/loginprompt.c: Remove dead code
20:  18b64250 = 11:  f5a2d873 lib/obscure.c: Mark parameter as [[maybe_unused]]
21:  e428fd7f = 12:  bdb71e5e autogen.sh: CFLAGS: Add -Wno-expansion-to-defined
22:  f3cd4318 = 13:  f0de1f5e src/login_nopam.c: Add missing 'const'
23:  f46b72cc = 14:  d0ea5cca src/gpasswd.c: Move if out of cpp conditional
24:  438274c5 = 15:  26e359b5 src/gpasswd.c: Mark failure() as [[noreturn]]
25:  19764e85 = 16:  0badba27 src/gpasswd.c: Simplify cpp conditional
26:  99162724 = 17:  0482a952 src/passwd.c: Reduce scope of cpp conditional
27:  19c9bae5 = 18:  1da15f35 src/passwd.c: Mark parameters as [[maybe_unused]]

alejandro-colomar · 2023-10-30T02:06:16Z

v2d changes:

Rebase to master

$ git range-diff 180160a0^..gh/wextra master..wextra 
 1:  180160a0 =  1:  7c95221f tests/unit/test_strlcpy.c: Test strlcpy_() and STRLCPY()
 2:  867f609b =  2:  a96f26ff lib/strlcpy.[ch]: Fix return type
 3:  942d7f5c =  3:  432f8f13 autogen.sh: CFLAGS: Add -Wextra
 4:  effb8500 =  4:  335cf7e9 lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning
 5:  3e836bcf =  5:  dc88ec66 lib/commonio.c: Use uintmax_t to print nlink_t
 6:  349934a0 =  6:  757b370e lib/strlcpy.h: Don't use a ternary op, to silence a -Wsign-compare warning
 7:  a668dfdd =  7:  8f30d4a0 lib/copydir.c: Cosmetic
 8:  8453864f =  8:  c9874098 lib/limits.c: Fix wrong error check
 9:  afa2200b =  9:  14b63aa7 lib/limits.c: Check for overflow without invoking UB
10:  6046bc95 = 10:  bd98c649 lib/loginprompt.c: Remove dead code
11:  f5a2d873 = 11:  44adfc6b lib/obscure.c: Mark parameter as [[maybe_unused]]
12:  bdb71e5e = 12:  48e40282 autogen.sh: CFLAGS: Add -Wno-expansion-to-defined
13:  f0de1f5e = 13:  eba829e4 src/login_nopam.c: Add missing 'const'
14:  d0ea5cca = 14:  23e72adc src/gpasswd.c: Move if out of cpp conditional
15:  26e359b5 = 15:  30314973 src/gpasswd.c: Mark failure() as [[noreturn]]
16:  0badba27 = 16:  e1b2685c src/gpasswd.c: Simplify cpp conditional
17:  0482a952 = 17:  ac48befc src/passwd.c: Reduce scope of cpp conditional
18:  1da15f35 = 18:  20180340 src/passwd.c: Mark parameters as [[maybe_unused]]

ikerexxe

First of all thank you for providing the unit tests for STRLCPY. I think it makes easier to understand its usage and to detect any failure.

Second, since you are changing is_valid_user_name() and it is a small function, would you mind adding a unit test for it?

Finally, what was the output of your investigation of a warning in copydir.c? I see you mention it in the commit message but no other action is taken.

By the way, I added some comments inline.

lib/commonio.c

lib/limits.c

alejandro-colomar · 2023-11-08T15:29:33Z

Finally, what was the output of your investigation of a warning in copydir.c? I see you mention it in the commit message but no other action is taken.

The warnings are below:

$ touch lib/copydir.c 
$ make
make  all-recursive
make[1]: Entering directory '/home/alx/src/shadow/shadow/wextra'
Making all in lib
make[2]: Entering directory '/home/alx/src/shadow/shadow/wextra/lib'
  CC       libshadow_la-copydir.lo
copydir.c: In function 'copy_special':
copydir.c:687:38: warning: unused parameter 'src' [-Wunused-parameter]
  687 | copy_special(const struct path_info *src, const struct path_info *dst,
      |              ~~~~~~~~~~~~~~~~~~~~~~~~^~~
copydir.c:688:19: warning: unused parameter 'reset_selinux' [-Wunused-parameter]
  688 |              bool reset_selinux,
      |                   ^
copydir.c: In function 'copy_file':
copydir.c:743:28: warning: unused parameter 'reset_selinux' [-Wunused-parameter]
  743 |                       bool reset_selinux,
      |                            ^

Analysis of copy_special():

$ grepc -ntfd copy_special .


./lib/copydir.c:686:
static int
copy_special(const struct path_info *src, const struct path_info *dst,
             bool reset_selinux,
             const struct stat *statp, const struct timespec mt[],
             uid_t old_uid, uid_t new_uid,
             gid_t old_gid, gid_t new_gid)
{
#if defined(WITH_SELINUX)
	if (set_selinux_file_context(dst->full_path, statp->st_mode & S_IFMT) != 0)
		return -1;
#endif

	if (mknodat(dst->dirfd, dst->name, statp->st_mode & ~07777U, statp->st_rdev) == -1)
		return -1;

	if (chownat_if_needed(dst, statp, old_uid, new_uid, old_gid, new_gid) == -1)
		return -1;

	if (fchmodat(dst->dirfd, dst->name, statp->st_mode & 07777, AT_SYMLINK_NOFOLLOW) == -1)
		return -1;

#if defined(WITH_ACL)
	if (perm_copy_path(src, dst, &ctx) == -1 && errno != 0)
		return -1;
#endif

#if defined(WITH_ATTR)
	/*
	 * If the third parameter is NULL, all extended attributes
	 * except those that define Access Control Lists are copied.
	 * ACLs are excluded by default because copying them between
	 * file systems with and without ACL support needs some
	 * additional logic so that no unexpected permissions result.
	 */
	if (!reset_selinux) {
		if (attr_copy_path(src, dst, NULL, &ctx) == -1 && errno != 0)
			return -1;
	}
#endif

	if (utimensat(dst->dirfd, dst->name, mt, AT_SYMLINK_NOFOLLOW) == -1)
		return -1;

	return 0;
}

Some parameters are unused depending on the configuration. We could use unused with those.

$ grepc -itm -C3 unused .
./lib/defines.h-
./lib/defines.h-/* To be used for verified unused parameters */
./lib/defines.h-#if defined(__GNUC__) && !defined(__STRICT_ANSI__)
./lib/defines.h:# define unused    __attribute__((unused))
./lib/defines.h-# define NORETURN  __attribute__((__noreturn__))
./lib/defines.h-# define format_attr(type, index, check) __attribute__((format (type, index, check)))
./lib/defines.h-#else
./lib/defines.h:# define unused
./lib/defines.h-# define NORETURN
./lib/defines.h-# define format_attr(type, index, check)
./lib/defines.h-#endif

Analysis of copy_file():

$ grepc -ntfd copy_file .


./lib/copydir.c:742:
static int copy_file (const struct path_info *src, const struct path_info *dst,
                      bool reset_selinux,
                      const struct stat *statp, const struct timespec mt[],
                      uid_t old_uid, uid_t new_uid,
                      gid_t old_gid, gid_t new_gid)
{
	int err = 0;
	int ifd;
	int ofd;

	ifd = openat (src->dirfd, src->name, O_RDONLY|O_NOFOLLOW|O_CLOEXEC);
	if (ifd < 0) {
		return -1;
	}
#ifdef WITH_SELINUX
	if (set_selinux_file_context (dst->full_path, S_IFREG) != 0) {
		(void) close (ifd);
		return -1;
	}
#endif				/* WITH_SELINUX */
	ofd = openat (dst->dirfd, dst->name, O_WRONLY | O_CREAT | O_EXCL | O_TRUNC | O_NOFOLLOW | O_CLOEXEC, 0600);
	if (   (ofd < 0)
	    || (fchown_if_needed (ofd, statp,
	                          old_uid, new_uid, old_gid, new_gid) != 0)
	    || (fchmod (ofd, statp->st_mode & 07777) != 0)
#ifdef WITH_ACL
	    || (   (perm_copy_fd (src->full_path, ifd, dst->full_path, ofd, &ctx) != 0)
	        && (errno != 0))
#endif				/* WITH_ACL */
#ifdef WITH_ATTR
	/*
	 * If the third parameter is NULL, all extended attributes
	 * except those that define Access Control Lists are copied.
	 * ACLs are excluded by default because copying them between
	 * file systems with and without ACL support needs some
	 * additional logic so that no unexpected permissions result.
	 */
	    || (   !reset_selinux
	        && (attr_copy_fd (src->full_path, ifd, dst->full_path, ofd, NULL, &ctx) != 0)
	        && (errno != 0))
#endif				/* WITH_ATTR */
	   ) {
		if (ofd >= 0) {
			(void) close (ofd);
		}
		(void) close (ifd);
		return -1;
	}

	while (true) {
		char buf[8192];
		ssize_t cnt;

		cnt = read (ifd, buf, sizeof buf);
		if (cnt < 0) {
			if (errno == EINTR) {
				continue;
			}
			(void) close (ofd);
			(void) close (ifd);
			return -1;
		}
		if (cnt == 0) {
			break;
		}

		if (write_full(ofd, buf, cnt) == -1) {
			(void) close (ofd);
			(void) close (ifd);
			return -1;
		}
	}

	(void) close (ifd);
	if (close (ofd) != 0) {
		return -1;
	}

	if (utimensat (dst->dirfd, dst->name, mt, AT_SYMLINK_NOFOLLOW) != 0) {
		return -1;
	}

	return err;
}

Same thing. Depending on the configuration.

alejandro-colomar · 2023-11-08T15:33:47Z

Second, since you are changing is_valid_user_name() and it is a small function, would you mind adding a unit test for it?

Yes, I can do that! :)

alejandro-colomar · 2023-11-12T23:00:10Z

I'll mark it as draft until that test is written.

alejandro-colomar · 2023-11-22T12:23:02Z

v3 changes:

Rebase to master
Update patch to strtcpy()

$ git range-diff master..gh/wextra shadow/master..wextra 
 1:  7c95221f <  -:  -------- tests/unit/test_strlcpy.c: Test strlcpy_() and STRLCPY()
 2:  a96f26ff <  -:  -------- lib/strlcpy.[ch]: Fix return type
 3:  432f8f13 =  1:  ad11b083 autogen.sh: CFLAGS: Add -Wextra
 4:  335cf7e9 =  2:  f1bf70a8 lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning
 5:  dc88ec66 =  3:  ef29f69f lib/commonio.c: Use uintmax_t to print nlink_t
 6:  757b370e <  -:  -------- lib/strlcpy.h: Don't use a ternary op, to silence a -Wsign-compare warning
 7:  8f30d4a0 =  4:  a61f033c lib/copydir.c: Cosmetic
 8:  c9874098 =  5:  d3488fb1 lib/limits.c: Fix wrong error check
 9:  14b63aa7 =  6:  803fef24 lib/limits.c: Check for overflow without invoking UB
10:  bd98c649 =  7:  f6fe6c5c lib/loginprompt.c: Remove dead code
11:  44adfc6b =  8:  9568b4b8 lib/obscure.c: Mark parameter as [[maybe_unused]]
12:  48e40282 =  9:  a076e741 autogen.sh: CFLAGS: Add -Wno-expansion-to-defined
13:  eba829e4 = 10:  8983bbf8 src/login_nopam.c: Add missing 'const'
14:  23e72adc = 11:  426f745e src/gpasswd.c: Move if out of cpp conditional
15:  30314973 = 12:  0da45b54 src/gpasswd.c: Mark failure() as [[noreturn]]
16:  e1b2685c = 13:  b8227f93 src/gpasswd.c: Simplify cpp conditional
17:  ac48befc = 14:  0c16fdcc src/passwd.c: Reduce scope of cpp conditional
18:  20180340 = 15:  0bb225d4 src/passwd.c: Mark parameters as [[maybe_unused]]
 -:  -------- > 16:  9ff74c17 lib/strtcpy.h: Don't use a ternary op, to silence a -Wsign-compare warning

alejandro-colomar · 2023-11-26T10:09:37Z

v4 changes:

Rebase to master. reuse() (from src/passwd.c) was removed in master, so drop related commits.

$ git range-diff master..gh/wextra shadow/master..wextra 
 1:  ad11b083 =  1:  ed46e928 autogen.sh: CFLAGS: Add -Wextra
 2:  f1bf70a8 =  2:  85b1649f lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning
 3:  ef29f69f =  3:  9f167093 lib/commonio.c: Use uintmax_t to print nlink_t
 4:  a61f033c =  4:  675b8430 lib/copydir.c: Cosmetic
 5:  d3488fb1 =  5:  550353ee lib/limits.c: Fix wrong error check
 6:  803fef24 =  6:  8ef4f59f lib/limits.c: Check for overflow without invoking UB
 7:  f6fe6c5c =  7:  33a124ff lib/loginprompt.c: Remove dead code
 8:  9568b4b8 =  8:  0ee9850f lib/obscure.c: Mark parameter as [[maybe_unused]]
 9:  a076e741 =  9:  2c2cdf31 autogen.sh: CFLAGS: Add -Wno-expansion-to-defined
10:  8983bbf8 = 10:  f7ea787a src/login_nopam.c: Add missing 'const'
11:  426f745e = 11:  54880aed src/gpasswd.c: Move if out of cpp conditional
12:  0da45b54 = 12:  3918e9fd src/gpasswd.c: Mark failure() as [[noreturn]]
13:  b8227f93 = 13:  5b702689 src/gpasswd.c: Simplify cpp conditional
14:  0c16fdcc <  -:  -------- src/passwd.c: Reduce scope of cpp conditional
15:  0bb225d4 <  -:  -------- src/passwd.c: Mark parameters as [[maybe_unused]]
16:  9ff74c17 = 14:  8387cf19 lib/strtcpy.h: Don't use a ternary op, to silence a -Wsign-compare warning

alejandro-colomar · 2023-11-26T11:57:16Z

v5 changes:

Fix bug introduced by accident in earlier versions of this PR. I accidentally inverted a cpp(1) conditional while changing from #ifdef to #if defined() while at the same time inverting the logic. I'll soon send a v6 where I split the patch into two, to do less things in each patch.

$ git range-diff master gh/wextra wextra 
 1:  ed46e928 =  1:  ed46e928 autogen.sh: CFLAGS: Add -Wextra
 2:  85b1649f =  2:  85b1649f lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning
 3:  9f167093 =  3:  9f167093 lib/commonio.c: Use uintmax_t to print nlink_t
 4:  675b8430 =  4:  675b8430 lib/copydir.c: Cosmetic
 5:  550353ee =  5:  550353ee lib/limits.c: Fix wrong error check
 6:  8ef4f59f =  6:  8ef4f59f lib/limits.c: Check for overflow without invoking UB
 7:  33a124ff =  7:  33a124ff lib/loginprompt.c: Remove dead code
 8:  0ee9850f =  8:  0ee9850f lib/obscure.c: Mark parameter as [[maybe_unused]]
 9:  2c2cdf31 =  9:  2c2cdf31 autogen.sh: CFLAGS: Add -Wno-expansion-to-defined
10:  f7ea787a = 10:  f7ea787a src/login_nopam.c: Add missing 'const'
11:  54880aed = 11:  54880aed src/gpasswd.c: Move if out of cpp conditional
12:  3918e9fd = 12:  3918e9fd src/gpasswd.c: Mark failure() as [[noreturn]]
13:  5b702689 ! 13:  2f5ec412 src/gpasswd.c: Simplify cpp conditional
    @@ src/gpasswd.c: static void check_perms (const struct group *gr)
                 * first group member might be just a normal user.
                 * --marekm
                 */
    -+#if defined(FIRST_MEMBER_IS_ADMIN)
    ++#if !defined(FIRST_MEMBER_IS_ADMIN)
     +          failure();
     +#endif
                if (gr->gr_mem[0] == NULL)
14:  8387cf19 = 14:  859e44a6 lib/strtcpy.h: Don't use a ternary op, to silence a -Wsign-compare warning

lib/limits.c

alejandro-colomar · 2023-11-30T08:57:05Z

v8 changes:

Revert to v6e. Drop the last commit.

hallyn · 2023-12-02T21:10:27Z

Thanks, looks good to me. Please add the one comment and i will merge.

alejandro-colomar · 2023-12-02T22:11:15Z

The one comment. :)

alejandro-colomar · 2023-12-03T17:39:26Z

v8b changes:

Rebased to master

$ git range-diff master..gh/wextra shadow/master..wextra 
 1:  17d83070 =  1:  b2a3d91b autogen.sh: CFLAGS: Add -Wextra
 2:  750a552f =  2:  9d7ea858 lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning
 3:  829db052 =  3:  f6540036 lib/commonio.c: Use uintmax_t to print nlink_t
 4:  6d5ad5c6 =  4:  e06e70c2 lib/copydir.c: Cosmetic
 5:  83af2525 =  5:  64f0a256 lib/limits.c: Fix wrong error check
 6:  29c11284 =  6:  f8d0fe07 lib/limits.c: Check for overflow without invoking UB
 7:  8ea11e05 =  7:  772986f9 lib/loginprompt.c: Remove dead code
 8:  475410a2 =  8:  d2ee150e lib/obscure.c: Mark parameter as [[maybe_unused]]
 9:  61a4d2b4 =  9:  a917987e autogen.sh: CFLAGS: Add -Wno-expansion-to-defined
10:  6ec1e78b = 10:  388993a8 src/login_nopam.c: Add missing 'const'
11:  e29861bc = 11:  1214f066 src/gpasswd.c: Move if out of cpp conditional
12:  42bc96d0 = 12:  85bdbfc8 src/gpasswd.c: Mark failure() as [[noreturn]]
13:  1f06c49a = 13:  b7c7112c src/gpasswd.c: Reduce scope of cpp conditional
14:  ff071879 = 14:  dc3b5905 src/gpasswd.c: Simplify cpp conditional
15:  efe6672f = 15:  77f63bda lib/strtcpy.h: Don't use a ternary op, to silence a -Wsign-compare warning

Signed-off-by: Alejandro Colomar <[email protected]>

I used size_t because: sysconf(3) can return -1 if the value is not supported, but then it can only mean that there's no limit. Having no limit is the same as having a limit of SIZE_MAX (to which -1 is converted). Signed-off-by: Alejandro Colomar <[email protected]>

See uintmax_t(3type). While at it, remove the useless cast to (void). Signed-off-by: Alejandro Colomar <[email protected]>

I was investigating a warning in this function, but the code was inscrutable. Signed-off-by: Alejandro Colomar <[email protected]>

strtol(3) doesn't specify a return value if (value == endptr). It is always an error, if (value==endptr). Signed-off-by: Alejandro Colomar <[email protected]>

The multiplication was already invoking UB. The test was flawed. Use __builtin_mul_overflow() instead. Signed-off-by: Alejandro Colomar <[email protected]>

Signed-off-by: Alejandro Colomar <[email protected]>

It's only used in certain builds. This is to silence a -Wunused-parameter warning. Signed-off-by: Alejandro Colomar <[email protected]>

Signed-off-by: Alejandro Colomar <[email protected]>

This simplifies the code a little bit, and prepares for the next commits, which will clean up further. Signed-off-by: Alejandro Colomar <[email protected]>

Signed-off-by: Alejandro Colomar <[email protected]>

This prepares for the next patch, which will invert the logic of the conditional. Signed-off-by: Alejandro Colomar <[email protected]>

Since failure() is [[noreturn]], we can invert the conditional so that we don't need an else. This silences a -Wunused-parameter warning. Signed-off-by: Alejandro Colomar <[email protected]>

…pare warning Signed-off-by: Alejandro Colomar <[email protected]>

alejandro-colomar · 2023-12-03T19:16:01Z

v9 changes:

Rebase to master (move to string/)

$ git range-diff master..gh/wextra shadow/master..wextra 
 1:  b2a3d91b =  1:  06d7e2e9 autogen.sh: CFLAGS: Add -Wextra
 2:  9d7ea858 =  2:  9b500285 lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning
 3:  f6540036 =  3:  29d232dc lib/commonio.c: Use uintmax_t to print nlink_t
 4:  e06e70c2 =  4:  55efab84 lib/copydir.c: Cosmetic
 5:  64f0a256 =  5:  79871384 lib/limits.c: Fix wrong error check
 6:  f8d0fe07 =  6:  c755b255 lib/limits.c: Check for overflow without invoking UB
 7:  772986f9 =  7:  a93f9363 lib/loginprompt.c: Remove dead code
 8:  d2ee150e =  8:  ead3262f lib/obscure.c: Mark parameter as [[maybe_unused]]
 9:  a917987e =  9:  5bf48a83 autogen.sh: CFLAGS: Add -Wno-expansion-to-defined
10:  388993a8 = 10:  3b394f27 src/login_nopam.c: Add missing 'const'
11:  1214f066 = 11:  0964affb src/gpasswd.c: Move if out of cpp conditional
12:  85bdbfc8 = 12:  3db434d9 src/gpasswd.c: Mark failure() as [[noreturn]]
13:  b7c7112c = 13:  79842dc4 src/gpasswd.c: Reduce scope of cpp conditional
14:  dc3b5905 = 14:  e98f2b12 src/gpasswd.c: Simplify cpp conditional
15:  77f63bda ! 15:  ecc8f172 lib/strtcpy.h: Don't use a ternary op, to silence a -Wsign-compare warning
    @@ Metadata
     Author: Alejandro Colomar <[email protected]>
     
      ## Commit message ##
    -    lib/strtcpy.h: Don't use a ternary op, to silence a -Wsign-compare warning
    +    lib/string/strtcpy.h: Don't use a ternary op, to silence a -Wsign-compare warning
     
         Signed-off-by: Alejandro Colomar <[email protected]>
     
    - ## lib/strtcpy.h ##
    -@@ lib/strtcpy.h: strtcpy(char *restrict dst, const char *restrict src, size_t dsize)
    + ## lib/string/strtcpy.h ##
    +@@ lib/string/strtcpy.h: strtcpy(char *restrict dst, const char *restrict src, size_t dsize)
        p = mempcpy(dst, src, dlen);
        *p = '\0';

ikerexxe

LGTM! Merging...

I used size_t because: sysconf(3) can return -1 if the value is not supported, but then it can only mean that there's no limit. Having no limit is the same as having a limit of SIZE_MAX (to which -1 is converted). Signed-off-by: Alejandro Colomar <[email protected]> Cherry-picked-from: 6be85b0 ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning") [alx: This is to cherry-pick the next commit without conflict] Link: <#801> Link: <#935> Cc: Serge Hallyn <[email protected]> Cc: Tobias Stoeckmann <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

alejandro-colomar force-pushed the wextra branch from daf99a0 to abf9119 Compare September 2, 2023 16:47

alejandro-colomar changed the title ~~with extra cheese~~ -Wextra: with extra cheese Sep 6, 2023

alejandro-colomar force-pushed the wextra branch from abf9119 to 316ee71 Compare September 6, 2023 10:04

alejandro-colomar force-pushed the wextra branch 2 times, most recently from 25f4911 to bd992d2 Compare October 4, 2023 17:06

alejandro-colomar force-pushed the wextra branch 3 times, most recently from 2fab6a8 to 69dbe8e Compare October 20, 2023 13:19

alejandro-colomar marked this pull request as ready for review October 20, 2023 13:23

alejandro-colomar marked this pull request as draft October 20, 2023 13:42

alejandro-colomar marked this pull request as ready for review October 20, 2023 13:52

alejandro-colomar force-pushed the wextra branch from 69dbe8e to c3e4de0 Compare October 20, 2023 13:52

alejandro-colomar force-pushed the wextra branch from c3e4de0 to 19c9bae Compare October 20, 2023 17:54

alejandro-colomar force-pushed the wextra branch from 19c9bae to 1da15f3 Compare October 20, 2023 19:10

alejandro-colomar force-pushed the wextra branch from 1da15f3 to 2018034 Compare October 30, 2023 02:05

ikerexxe reviewed Nov 8, 2023

View reviewed changes

lib/commonio.c Show resolved Hide resolved

lib/limits.c Show resolved Hide resolved

alejandro-colomar marked this pull request as draft November 12, 2023 23:00

alejandro-colomar force-pushed the wextra branch from 2018034 to 9ff74c1 Compare November 22, 2023 12:20

alejandro-colomar force-pushed the wextra branch from 9ff74c1 to 8387cf1 Compare November 26, 2023 10:08

alejandro-colomar force-pushed the wextra branch from 8387cf1 to 859e44a Compare November 26, 2023 11:54

ikerexxe reviewed Nov 30, 2023

View reviewed changes

lib/limits.c Outdated Show resolved Hide resolved

alejandro-colomar force-pushed the wextra branch from 643b176 to efe6672 Compare November 30, 2023 08:56

alejandro-colomar mentioned this pull request Dec 1, 2023

Fix error checking after strto[u]l[l](3) #856

Merged

alejandro-colomar force-pushed the wextra branch from efe6672 to 77f63bd Compare December 3, 2023 17:38

alejandro-colomar added 15 commits December 3, 2023 20:14

autogen.sh: CFLAGS: Add -Wextra

06d7e2e

Signed-off-by: Alejandro Colomar <[email protected]>

lib/commonio.c: Use uintmax_t to print nlink_t

29d232d

See uintmax_t(3type). While at it, remove the useless cast to (void). Signed-off-by: Alejandro Colomar <[email protected]>

lib/copydir.c: Cosmetic

55efab8

I was investigating a warning in this function, but the code was inscrutable. Signed-off-by: Alejandro Colomar <[email protected]>

lib/limits.c: Fix wrong error check

7987138

strtol(3) doesn't specify a return value if (value == endptr). It is always an error, if (value==endptr). Signed-off-by: Alejandro Colomar <[email protected]>

lib/limits.c: Check for overflow without invoking UB

c755b25

The multiplication was already invoking UB. The test was flawed. Use __builtin_mul_overflow() instead. Signed-off-by: Alejandro Colomar <[email protected]>

lib/loginprompt.c: Remove dead code

a93f936

Signed-off-by: Alejandro Colomar <[email protected]>

lib/obscure.c: Mark parameter as [[maybe_unused]]

ead3262

It's only used in certain builds. This is to silence a -Wunused-parameter warning. Signed-off-by: Alejandro Colomar <[email protected]>

autogen.sh: CFLAGS: Add -Wno-expansion-to-defined

5bf48a8

Signed-off-by: Alejandro Colomar <[email protected]>

src/login_nopam.c: Add missing 'const'

3b394f2

Signed-off-by: Alejandro Colomar <[email protected]>

src/gpasswd.c: Move if out of cpp conditional

0964aff

This simplifies the code a little bit, and prepares for the next commits, which will clean up further. Signed-off-by: Alejandro Colomar <[email protected]>

src/gpasswd.c: Mark failure() as [[noreturn]]

3db434d

Signed-off-by: Alejandro Colomar <[email protected]>

src/gpasswd.c: Reduce scope of cpp conditional

79842dc

This prepares for the next patch, which will invert the logic of the conditional. Signed-off-by: Alejandro Colomar <[email protected]>

src/gpasswd.c: Simplify cpp conditional

e98f2b1

Since failure() is [[noreturn]], we can invert the conditional so that we don't need an else. This silences a -Wunused-parameter warning. Signed-off-by: Alejandro Colomar <[email protected]>

lib/string/strtcpy.h: Don't use a ternary op, to silence a -Wsign-com…

ecc8f17

…pare warning Signed-off-by: Alejandro Colomar <[email protected]>

alejandro-colomar force-pushed the wextra branch from 77f63bd to ecc8f17 Compare December 3, 2023 19:15

ikerexxe approved these changes Dec 4, 2023

View reviewed changes

ikerexxe merged commit ad1e0e9 into shadow-maint:master Dec 4, 2023
9 checks passed

alejandro-colomar deleted the wextra branch December 4, 2023 11:05

alejandro-colomar mentioned this pull request Feb 4, 2024

4.14.4 stable release #920

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

-Wextra: with extra cheese #801

-Wextra: with extra cheese #801

alejandro-colomar commented Sep 2, 2023 •

edited

Loading

alejandro-colomar commented Oct 20, 2023 •

edited

Loading

alejandro-colomar commented Oct 20, 2023 •

edited

Loading

alejandro-colomar commented Oct 20, 2023

alejandro-colomar commented Oct 30, 2023 •

edited

Loading

ikerexxe left a comment

alejandro-colomar commented Nov 8, 2023

alejandro-colomar commented Nov 8, 2023

alejandro-colomar commented Nov 12, 2023

alejandro-colomar commented Nov 22, 2023

alejandro-colomar commented Nov 26, 2023 •

edited

Loading

alejandro-colomar commented Nov 26, 2023 •

edited

Loading

alejandro-colomar commented Nov 30, 2023

hallyn commented Dec 2, 2023

alejandro-colomar commented Dec 2, 2023

alejandro-colomar commented Dec 3, 2023

alejandro-colomar commented Dec 3, 2023

ikerexxe left a comment

-Wextra: with extra cheese #801

-Wextra: with extra cheese #801

Conversation

alejandro-colomar commented Sep 2, 2023 • edited Loading

alejandro-colomar commented Oct 20, 2023 • edited Loading

alejandro-colomar commented Oct 20, 2023 • edited Loading

alejandro-colomar commented Oct 20, 2023

alejandro-colomar commented Oct 30, 2023 • edited Loading

ikerexxe left a comment

Choose a reason for hiding this comment

alejandro-colomar commented Nov 8, 2023

alejandro-colomar commented Nov 8, 2023

alejandro-colomar commented Nov 12, 2023

alejandro-colomar commented Nov 22, 2023

alejandro-colomar commented Nov 26, 2023 • edited Loading

alejandro-colomar commented Nov 26, 2023 • edited Loading

alejandro-colomar commented Nov 30, 2023

hallyn commented Dec 2, 2023

alejandro-colomar commented Dec 2, 2023

alejandro-colomar commented Dec 3, 2023

alejandro-colomar commented Dec 3, 2023

ikerexxe left a comment

Choose a reason for hiding this comment

alejandro-colomar commented Sep 2, 2023 •

edited

Loading

alejandro-colomar commented Oct 20, 2023 •

edited

Loading

alejandro-colomar commented Oct 20, 2023 •

edited

Loading

alejandro-colomar commented Oct 30, 2023 •

edited

Loading

alejandro-colomar commented Nov 26, 2023 •

edited

Loading

alejandro-colomar commented Nov 26, 2023 •

edited

Loading