search

openwall / tcb

Alternative password shadowing scheme
https://www.openwall.com/tcb/
Other
8 stars 3 forks source link

Fix several advanced compiler warnings. #8

Closed besser82 closed 3 years ago

besser82 commented 3 years ago

pam_tcb: Fix -Wpedantic.

besser82 commented 3 years ago

This one is ready, and should go first.

solardiz commented 3 years ago

Let's discuss first: is there a reason to avoid GNU extensions in tcb? I suppose currently it makes sense to build tcb with gcc and clang, but I think the latter also supports these extensions? And can we assume that %m is supported by the libraries?

I suppose a reason to avoid these would be if we e.g. intend to get pam_tcb into Linux-PAM and the conventions there are not to use such extensions (I don't know if that's the case or not).

besser82 commented 3 years ago

I just check the Linux-PAM repository and it seems %m is used frequently there, while I couldn't spot any use of the shortened ?: ternary in there.

besser82 commented 3 years ago

In there rebased commits I've dropped the change about %mand added a small static inlineto solve the ?:shortned ternary.

ldv-alt commented 3 years ago

I just check the Linux-PAM repository and it seems %m is used frequently there, while I couldn't spot any use of the shortened ?: ternary in there.

Most of %m in Linux-PAM are used along with syslog and pam_syslog where %m is a valid format specifier. There are few cases where %m is used in printf format strings, though:

$ git grep -F '%m\n'
modules/pam_faillock/main.c:        fprintf(stderr, "%s: Error reading tally directory: %m\n", opts->progname);
modules/pam_umask/pam_umask.c:      pam_error (pamh, "nice failed: %m\n");
modules/pam_umask/pam_umask.c:      pam_error (pamh, "setrlimit failed: %m\n");
xtests/tst-pam_group1.c:        fprintf (stderr, "pam_group1: getroups returned error: %m\n");

In tcb, %m is used only with syslog and pam_syslog, so there is nothing to fix about it.

besser82 commented 3 years ago

@ldv-alt Please have second look on this PR. I hope you're fine with it now.

solardiz commented 3 years ago

I agree with these changes:

    * Make.defs: Add "-Wextra" to default CFLAGS.  Add an option to
    enable "-Werror" also.
    * ci/run-build-and-tests.sh: Build with "-Werror" enabled on CI.

I am still not convinced that this is an overall improvement, but I don't really mind:

    pam_tcb: Fix "-Wpedantic".
    * pam_tcb/pam_unix_auth.c (pam_sm_authenticate): ISO C forbids
    omitting the middle term of a '?:' expression.
    * pam_tcb/pam_unix_sess.c (pam_sm_open_session): Likewise.
    * pam_tcb/pam_unix_passwd.c (pam_sm_chauthtok): Likewise.
    * pam_tcb/pam_unix_passwd.c (unix_prelim): Likewise.
    * pam_tcb/support.c (_set_ctrl): Likewise.
    * pam_tcb/support.h (pam_tcb_getlogin): New function.
    Small static inline wrapper around getlogin(3).

My reasoning is that if GNU extensions are ever to be used at all, then system-specific projects like tcb are the right ones to use them. Are there any Open Source operating systems that would potentially integrate our tcb yet not support basic GNU C extensions like this? In most other Openwall software projects, sure we avoid hard dependencies on GNU extensions.

Anyway, like I said I don't mind, so I'll defer to @besser82 and @ldv-alt on this.

besser82 commented 3 years ago

My reasoning is that if GNU extensions are ever to be used at all, then system-specific projects like tcb are the right ones to use them. Are there any Open Source operating systems that would potentially integrate our tcb yet not support basic GNU C extensions like this? In most other Openwall software projects, sure we avoid hard dependencies on GNU extensions.

From my POV I usually like to keep code as portable as possible. While %m format is fairly portable in the UNIX landscape, there are likely some compilers around - even in the Linux universe - that do not support shortened ?: ternary operators.

ldv-alt commented 3 years ago

I don't mind GNU extensions in general, and I like ?: because it's more expressive. That said, I don't object against this change.

solardiz commented 3 years ago

I was wondering about possible regressions with older gcc, but building @besser82's branch from this PR with RHEL6's gcc 4.4.7 didn't result in any additional warnings. (I only got 3 warnings for missing prototypes for our crypt_* extensions, which RHEL6 obviously doesn't have.)

Given your comments, I'll go ahead and merge this, even though I still think the usage of ?: in tcb was appropriate. (My original thinking back then included the fact that to build tcb one needed glibc+crypt_blowfish anyway, and building glibc obviously requires a GNU C capable compiler, and a core package like tcb would be built in a distro by the same compiler as glibc. I guess similar logic probably holds for systems with libxcrypt now. But whatever.)