Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 19 Jan 2012 12:54:54 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Peter Wemm <peter@FreeBSD.org>
Cc:        svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org
Subject:   Re: svn commit: r230311 - head/lib/libpam/modules/pam_unix
Message-ID:  <20120119110628.F1702@besplex.bde.org>
In-Reply-To: <201201181826.q0IIQuLX051824@svn.freebsd.org>
References:  <201201181826.q0IIQuLX051824@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 18 Jan 2012, Peter Wemm wrote:

> Log:
>  Rev 228065 (change bsd.own.mk -> bsd.init.mk) broke pam_unix.so by causing
>  the LDADD/DPADD to lose the -lpam, and causing openpam_dynamic() to fail
>  due to "openpam_get_options" being undefined.

bsd.init.mk (actually this abuse of it -- see below) is horribily
broken.  It includes ../Makefile.inc, so anything that includes it
early tends to get ../Makefile.inc in a wrong order.  The normal order
is to include bsd.prog.mk late, and get ../Makefile.inc late from
there.  Any early include of a bsd.*.mk file is dangerous because it
is not the normal order.  Unfortunately, a few Makefiles need to include
bsd.own.mk early.  bsd.init.mk includes bsd.own.mk, so these Makefiles
can now (actually for a long time) include bsd.init.mk instead.

I think bsd.init.mk is an implementation detail and including it
directly is just bug.  Its documentation still agrees with this
interpretatation:
- the comment at the start of it only says that it is used at the top
   of all <bsd.*.mk> files.
- the automatic include of ../Makefile.inc is only documented (in
   bsd.README) for <bsd.man.mk>, <bsd.prog.mk> and <bsd.lib.mk>.  BTW,
   why the bracket quoting for these?
- bsd.README documents bsd.init.mk as being for the make include files.

In /usr/src on Nov 13 2011, the only Makefile[.inc] files with this bug
are:
   gnu/usr.bin/cc/include/Makefile
   lib/clang/include/Makefile
   sys/boot/toomany/Makefile   (this matches the general low quality of
     boot Makefiles)
The bug in libpam was too new to show up here.

>
>  This would cause obscure console log messages like:
>    openpam_dynamic(): No error: 0
>    openpam_load_module(): no pam_unix.so found
>  and other helpful messages which are no help in diagnosing the problem.
>
>  Fortunately this change was not mfc'ed to 9.x, it isn't broken there.
>
> Modified:
>  head/lib/libpam/modules/pam_unix/Makefile
>
> Modified: head/lib/libpam/modules/pam_unix/Makefile
> ==============================================================================
> --- head/lib/libpam/modules/pam_unix/Makefile	Wed Jan 18 18:22:25 2012	(r230310)
> +++ head/lib/libpam/modules/pam_unix/Makefile	Wed Jan 18 18:26:56 2012	(r230311)
> @@ -40,8 +40,8 @@ LIB=	pam_unix
> SRCS=	pam_unix.c
> MAN=	pam_unix.8
>
> -DPADD= ${LIBUTIL} ${LIBCRYPT}
> -LDADD= -lutil -lcrypt
> +DPADD+= ${LIBUTIL} ${LIBCRYPT}
> +LDADD+= -lutil -lcrypt

Bitrot near here started in 2004 when the tabs were lost.

>
> .if ${MK_NIS} != "no"

Including bsd.own.mk is unfortunately necessary for getting MK_NIS
defined (bsd.own.mk is abused to define MK_* and compatibility cruft
that are both completely unrelated to ownership).  That is enough
for the "few" Makefiles that test MK_*.  (More than 200 Makefiles
include bsd.own.mk explicitly, presumably only (early) for testimg
MK_* :-(.)

According to the log message for the commit that broke this, bsd.init.mk
is used (early) instead of bsd.own.mk (early) to allow use of settings
from ../Makefile.inc.  This seems to be actually to use settings in
the user's make.conf or environment or defaults to kill CTF in an old
implementation of killing CTF.  Now, bsd.own.mk is abused further to
kill CTF, and I can't see why it doesn't do this when only it is
included early.

> CFLAGS+= -DYP
>

I don't like the structure of this file or tree at all.  The library
order is important for static linkage, so the implementation detail
that bsd.init.mk includes ../Makefile.inc is important (it only does
this for the benefit of bsd.prog.mk etc. which are documented to do
it.  According to a strict reading of the documentation, only bsd.prog.mk
does it, so the order is wrong here).

Actually, ../Makefile.inc has always added -lpam, but before this included
bsd.init.mk, its order was precisely the opposite of what it is now -- it
was last instead of first.  It's not clear how either order can work.
-lpam first seems right (in case this library refers to symbols in the
others), but the old order seemed to work.

I checked all the other includes of bsd.init.mk.  All except the boot
Makefiles use it correctly (near the end, for the technical reason that
they include something like bsd.obj.mk and not bsd.prog.mk, so they
don't include it in the normal way via bsd.prog.mk).  The boot makefiles
document that they are using it to get ../Makefile.inc.  They should
just include ../Makefile.inc for that.  They need it mainly to get the
BTX* symbols defined.

Related bugs:
- sys.mk was polluted in 2004 with an unconditional include of
   bsd.compat.mk, just after the place where previous FreeBSD-build-
   specific includes are ifdefed to reduce such pollution.
- the include of bsd.compat.mk in bsd.init.mk has no effect, since
   sys.mk has already done it unconditionally.
- MK_CTF is still used unitialized kern.post.mk and kern.pre.mk.  It
   is uninitialized when the kernel source tree is current and the
   host mk non-kernel files more than a couple of months old.  The
   corresponding bug in kmod.mk was only there for a few days.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120119110628.F1702>