diff mbox

internal_function symbols part of the public ABI

Message ID 3be67f5e-c8ab-5937-ba48-6d1cd781414f@redhat.com
State New
Headers show

Commit Message

Florian Weimer Aug. 14, 2017, 3:28 p.m. UTC
On 08/14/2017 04:37 PM, Andreas Schwab wrote:
> On Aug 14 2017, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> The 2007 commit radically changed the calling convention, so it's safe
>> to assume that there were no active users on i386 at the time because
>> they would have run into crashes.  Today, it's still difficult to call
>> these functions because it is hard to write a correct prototype.
> 
> They are really private interfaces for nscd, AFAICS.
> 
>> Can we remove the functions completely?
> 
> They should be unexported, for sure.  Since nscd doesn't use them any
> more, they could just be dummies.

What about the attached patch?

I cleaned up the declarations of the __nss_*_lookup* functions.  They
are now generated from databases.def in <nsswitch.h>.  I switched all
the internal users of the *lookup functions to *lookup2 functions, so
that the compat functions are not used internally anymore.

Thanks,
Florian

Comments

Andreas Schwab Aug. 14, 2017, 3:47 p.m. UTC | #1
On Aug 14 2017, Florian Weimer <fweimer@redhat.com> wrote:

> 2017-08-14  Florian Weimer  <fweimer@redhat.com>
>
> 	[BZ #21962]
> 	NSS: Create stubs for accidentally exported lookup functions.
> 	* grp/initgroups.c (__nss_group_lookup, __nss_lookup_function):
> 	Remove declaration.
> 	* inet/ether_hton.c (__nss_ethers_lookup): Likewise.
> 	(ether_hostton): Call __nss_ethers_lookup2 instead.
> 	* inet/ether_ntoh.c (__nss_ethers_lookup): Remove declaration.
> 	(ether_ntohost): Call __nss_ethers_lookup2 instead.
> 	* inet/getnetgrent_r.c (__nss_netgroup_lookup): Remove declaration.
> 	(setup): Call __nss_netgroup_lookup2 instead.
> 	* nss/Makefile (routines): Add compat-lookup.
> 	* nss/Versions (GLIBC_2.27): Add symbol version.
> 	* nss/XXX-lookup (DB_LOOKUP_FCT): Remove declaration.  Now provided by <nsswitch.h>.
> 	(DB_COMPAT_FCT): Remove.
> 	* nss/compat-lookup.c: New file.
> 	* nss/nsswitch.h: Generate __nss_*_lookup2 function prototypes
> 	from databases.def.
> 	* nss/service-lookup.c (NO_COMPAT): Remove definition.
> 	* sunrpc/netname.c (__nss_publickey_lookup): Remove declaration.
> 	(netname2user): Call __nss_publickey_lookup2 instead.
> 	* sunrpc/publickey.c (__nss_publickey_lookup): Remove declaration.
> 	(getpublickey, getsecretkey): Call __nss_publickey_lookup2
> 	instead.

Ok.

Andreas.
Carlos O'Donell Aug. 14, 2017, 4:23 p.m. UTC | #2
On 08/14/2017 11:28 AM, Florian Weimer wrote:
> 2017-08-14  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #21962]
> 	NSS: Create stubs for accidentally exported lookup functions.
> 	* grp/initgroups.c (__nss_group_lookup, __nss_lookup_function):
> 	Remove declaration.
> 	* inet/ether_hton.c (__nss_ethers_lookup): Likewise.
> 	(ether_hostton): Call __nss_ethers_lookup2 instead.
> 	* inet/ether_ntoh.c (__nss_ethers_lookup): Remove declaration.
> 	(ether_ntohost): Call __nss_ethers_lookup2 instead.
> 	* inet/getnetgrent_r.c (__nss_netgroup_lookup): Remove declaration.
> 	(setup): Call __nss_netgroup_lookup2 instead.
> 	* nss/Makefile (routines): Add compat-lookup.
> 	* nss/Versions (GLIBC_2.27): Add symbol version.
> 	* nss/XXX-lookup (DB_LOOKUP_FCT): Remove declaration.  Now provided by <nsswitch.h>.
> 	(DB_COMPAT_FCT): Remove.
> 	* nss/compat-lookup.c: New file.
> 	* nss/nsswitch.h: Generate __nss_*_lookup2 function prototypes
> 	from databases.def.
> 	* nss/service-lookup.c (NO_COMPAT): Remove definition.
> 	* sunrpc/netname.c (__nss_publickey_lookup): Remove declaration.
> 	(netname2user): Call __nss_publickey_lookup2 instead.
> 	* sunrpc/publickey.c (__nss_publickey_lookup): Remove declaration.
> 	(getpublickey, getsecretkey): Call __nss_publickey_lookup2
> 	instead.

This looks good to me.

This is a great cleanup of a bunch of crufty functions that we had around
for no real reason other than lack of time for cleanup.

We should be using new nscd with new glibc anyway and distros have been
enforcing that for years.

The only slight worry I have is that there were rumours of NSS service
modules using these interfaces, particularly during early bootup to
avoid lookup issues while services came online. I have never actually
seen such code, and I don't think we should make allowances for it.

Cheers,
Carlos.
diff mbox

Patch

NSS: Replace exported NSS lookup functions with stubs [BZ #21962]

Commit 384ca551743318bd9c9e24a496d6397f2e3f2a49 from 2007 added this to
nss/XXX-lookup.c:

+#ifndef NO_COMPAT
+int
+internal_function attribute_compat_text_section
+DB_COMPAT_FCT (service_user **ni, const char *fct_name, void **fctp)
+{
+  return DB_LOOKUP_FCT (ni, fct_name, NULL, fctp);
+}
+#endif

That is, it adds a pseudo-compat function with an internal_function
attribute.  The function it was supposed to replace did not have the
attribute:

 extern int DB_LOOKUP_FCT (service_user **ni, const char *fct_name,
-			  void **fctp) internal_function;
+			  const char *fct2_name, void **fctp)
+  internal_function;

This changed the calling convention on i386 for the following
functions in the public ABI:

  __nss_passwd_lookup
  __nss_group_lookup
  __nss_hosts_lookup

This commit replaces the functions with always-failing stubs,
with true compat symbols.  Due to a happy accident, the calling
convention of the stub is identical for the internal_function
and non-internal_function case on i386.

In addition, this commit auto-generates the __nss_*_lookup2
function declarations as part of <nsswitch.h>.

2017-08-14  Florian Weimer  <fweimer@redhat.com>

	[BZ #21962]
	NSS: Create stubs for accidentally exported lookup functions.
	* grp/initgroups.c (__nss_group_lookup, __nss_lookup_function):
	Remove declaration.
	* inet/ether_hton.c (__nss_ethers_lookup): Likewise.
	(ether_hostton): Call __nss_ethers_lookup2 instead.
	* inet/ether_ntoh.c (__nss_ethers_lookup): Remove declaration.
	(ether_ntohost): Call __nss_ethers_lookup2 instead.
	* inet/getnetgrent_r.c (__nss_netgroup_lookup): Remove declaration.
	(setup): Call __nss_netgroup_lookup2 instead.
	* nss/Makefile (routines): Add compat-lookup.
	* nss/Versions (GLIBC_2.27): Add symbol version.
	* nss/XXX-lookup (DB_LOOKUP_FCT): Remove declaration.  Now provided by <nsswitch.h>.
	(DB_COMPAT_FCT): Remove.
	* nss/compat-lookup.c: New file.
	* nss/nsswitch.h: Generate __nss_*_lookup2 function prototypes
	from databases.def.
	* nss/service-lookup.c (NO_COMPAT): Remove definition.
	* sunrpc/netname.c (__nss_publickey_lookup): Remove declaration.
	(netname2user): Call __nss_publickey_lookup2 instead.
	* sunrpc/publickey.c (__nss_publickey_lookup): Remove declaration.
	(getpublickey, getsecretkey): Call __nss_publickey_lookup2
	instead.

diff --git a/grp/initgroups.c b/grp/initgroups.c
index 7a40813d5e..0d5b841796 100644
--- a/grp/initgroups.c
+++ b/grp/initgroups.c
@@ -36,11 +36,6 @@  typedef enum nss_status (*initgroups_dyn_function) (const char *, gid_t,
 						    long int *, long int *,
 						    gid_t **, long int, int *);
 
-/* The lookup function for the first entry of this service.  */
-extern int __nss_group_lookup (service_user **nip, const char *name,
-				   void **fctp);
-extern void *__nss_lookup_function (service_user *ni, const char *fct_name);
-
 extern service_user *__nss_group_database attribute_hidden;
 service_user *__nss_initgroups_database;
 static bool use_initgroups_entry;
diff --git a/inet/ether_hton.c b/inet/ether_hton.c
index a5523986c9..164838ecec 100644
--- a/inet/ether_hton.c
+++ b/inet/ether_hton.c
@@ -27,11 +27,6 @@ 
 typedef int (*lookup_function) (const char *, struct etherent *, char *, int,
 				int *);
 
-/* The lookup function for the first entry of this service.  */
-extern int __nss_ethers_lookup (service_user **nip, const char *name,
-				void **fctp) internal_function;
-
-
 int
 ether_hostton (const char *hostname, struct ether_addr *addr)
 {
@@ -49,7 +44,7 @@  ether_hostton (const char *hostname, struct ether_addr *addr)
 
   if (startp == NULL)
     {
-      no_more = __nss_ethers_lookup (&nip, "gethostton_r", &fct.ptr);
+      no_more = __nss_ethers_lookup2 (&nip, "gethostton_r", NULL, &fct.ptr);
       if (no_more)
 	startp = (service_user *) -1;
       else
diff --git a/inet/ether_ntoh.c b/inet/ether_ntoh.c
index b2f7422ace..393601ee7a 100644
--- a/inet/ether_ntoh.c
+++ b/inet/ether_ntoh.c
@@ -28,11 +28,6 @@ 
 typedef int (*lookup_function) (const struct ether_addr *, struct etherent *,
 				char *, size_t, int *);
 
-/* The lookup function for the first entry of this service.  */
-extern int __nss_ethers_lookup (service_user **nip, const char *name,
-				void **fctp) internal_function;
-
-
 int
 ether_ntohost (char *hostname, const struct ether_addr *addr)
 {
@@ -50,7 +45,7 @@  ether_ntohost (char *hostname, const struct ether_addr *addr)
 
   if (startp == NULL)
     {
-      no_more = __nss_ethers_lookup (&nip, "getntohost_r", &fct.ptr);
+      no_more = __nss_ethers_lookup2 (&nip, "getntohost_r", NULL, &fct.ptr);
       if (no_more)
 	startp = (service_user *) -1;
       else
diff --git a/inet/getnetgrent_r.c b/inet/getnetgrent_r.c
index a8fc51c1b6..89fefeb612 100644
--- a/inet/getnetgrent_r.c
+++ b/inet/getnetgrent_r.c
@@ -36,10 +36,6 @@  __libc_lock_define_initialized (static, lock)
    kept in this structure.  */
 static struct __netgrent dataset;
 
-/* The lookup function for the first entry of this service.  */
-extern int __nss_netgroup_lookup (service_user **nipp, const char *name,
-				  void **fctp) internal_function;
-
 /* Set up NIP to run through the services.  Return nonzero if there are no
    services (left).  */
 static int
@@ -54,7 +50,7 @@  setup (void **fctp, service_user **nipp)
     {
       /* Executing this more than once at the same time must yield the
 	 same result every time.  So we need no locking.  */
-      no_more = __nss_netgroup_lookup (nipp, "setnetgrent", fctp);
+      no_more = __nss_netgroup_lookup2 (nipp, "setnetgrent", NULL, fctp);
       startp = no_more ? (service_user *) -1 : *nipp;
 #ifdef PTR_MANGLE
       PTR_MANGLE (startp);
diff --git a/nss/Makefile b/nss/Makefile
index d9f6d41181..1e298c28f1 100644
--- a/nss/Makefile
+++ b/nss/Makefile
@@ -27,7 +27,8 @@  headers			:= nss.h
 # This is the trivial part which goes into libc itself.
 routines		= nsswitch getnssent getnssent_r digits_dots \
 			  valid_field valid_list_field rewrite_field \
-			  $(addsuffix -lookup,$(databases))
+			  $(addsuffix -lookup,$(databases)) \
+			  compat-lookup
 
 # These are the databases that go through nss dispatch.
 # Caution: if you add a database here, you must add its real name
diff --git a/nss/Versions b/nss/Versions
index f8ababccc7..50268ed9b5 100644
--- a/nss/Versions
+++ b/nss/Versions
@@ -7,6 +7,8 @@  libc {
   GLIBC_2.2.2 {
     __nss_hostname_digits_dots;
   }
+  GLIBC_2.27 {
+  }
   GLIBC_PRIVATE {
     _nss_files_parse_grent; _nss_files_parse_pwent; _nss_files_parse_spent;
     __nss_disable_nscd; __nss_lookup_function; _nss_files_parse_sgent;
diff --git a/nss/XXX-lookup.c b/nss/XXX-lookup.c
index 972a2102bf..49417691b2 100644
--- a/nss/XXX-lookup.c
+++ b/nss/XXX-lookup.c
@@ -34,7 +34,6 @@ 
 \*******************************************************************/
 
 #define DB_LOOKUP_FCT CONCAT3_1 (__nss_, DATABASE_NAME, _lookup2)
-#define DB_COMPAT_FCT CONCAT3_1 (__nss_, DATABASE_NAME, _lookup)
 #define CONCAT3_1(Pre, Name, Post) CONCAT3_2 (Pre, Name, Post)
 #define CONCAT3_2(Pre, Name, Post) Pre##Name##Post
 
@@ -55,10 +54,6 @@ 
 
 service_user *DATABASE_NAME_SYMBOL attribute_hidden;
 
-extern int DB_LOOKUP_FCT (service_user **ni, const char *fct_name,
-			  const char *fct2_name, void **fctp);
-libc_hidden_proto (DB_LOOKUP_FCT)
-
 int
 DB_LOOKUP_FCT (service_user **ni, const char *fct_name, const char *fct2_name,
 	       void **fctp)
@@ -73,13 +68,3 @@  DB_LOOKUP_FCT (service_user **ni, const char *fct_name, const char *fct2_name,
   return __nss_lookup (ni, fct_name, fct2_name, fctp);
 }
 libc_hidden_def (DB_LOOKUP_FCT)
-
-
-#ifndef NO_COMPAT
-int
-attribute_compat_text_section
-DB_COMPAT_FCT (service_user **ni, const char *fct_name, void **fctp)
-{
-  return DB_LOOKUP_FCT (ni, fct_name, NULL, fctp);
-}
-#endif
diff --git a/nss/compat-lookup.c b/nss/compat-lookup.c
new file mode 100644
index 0000000000..b5c9673b49
--- /dev/null
+++ b/nss/compat-lookup.c
@@ -0,0 +1,42 @@ 
+/* Compatibility stubs of accidentally exported __nss_*_lookup functions.
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <shlib-compat.h>
+#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_27)
+
+# include <errno.h>
+# include <nsswitch.h>
+
+/* On i386, the function calling convention changed from the standard
+   ABI calling convention to three register parameters in glibc 2.8.
+   The following error-returning stub happens to be compatible with
+   glibc 2.7 and earlier and glibc 2.8 and later, even on i386.  */
+int
+attribute_compat_text_section
+__nss_passwd_lookup (service_user **ni, const char *fct_name, void **fctp)
+{
+  __set_errno (ENOSYS);
+  return -1;
+}
+compat_symbol (libc, __nss_passwd_lookup, __nss_passwd_lookup, GLIBC_2_0);
+strong_alias (__nss_passwd_lookup, __nss_group_lookup)
+compat_symbol (libc, __nss_group_lookup, __nss_group_lookup, GLIBC_2_0);
+strong_alias (__nss_passwd_lookup, __nss_hosts_lookup)
+compat_symbol (libc, __nss_hosts_lookup, __nss_hosts_lookup, GLIBC_2_0);
+
+#endif /* SHLIB_COMPAT */
diff --git a/nss/nsswitch.h b/nss/nsswitch.h
index 7da90130ec..2b86d63ddb 100644
--- a/nss/nsswitch.h
+++ b/nss/nsswitch.h
@@ -219,4 +219,12 @@  libc_hidden_proto (__nss_hostname_digits_dots)
 #define MAX_NR_ALIASES  48
 #define MAX_NR_ADDRS    48
 
+/* Prototypes for __nss_*_lookup2 functions.  */
+#define DEFINE_DATABASE(arg)				    \
+  int __nss_##arg##_lookup2 (service_user **, const char *, \
+			     const char *, void **);	    \
+  libc_hidden_proto (__nss_##arg##_lookup2)
+#include "databases.def"
+#undef DEFINE_DATABASE
+
 #endif	/* nsswitch.h */
diff --git a/nss/service-lookup.c b/nss/service-lookup.c
index 3f230d606a..3b97b419b6 100644
--- a/nss/service-lookup.c
+++ b/nss/service-lookup.c
@@ -17,6 +17,5 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 #define DATABASE_NAME services
-#define NO_COMPAT
 
 #include "XXX-lookup.c"
diff --git a/sunrpc/netname.c b/sunrpc/netname.c
index 9aee3e4042..0dacd1cac7 100644
--- a/sunrpc/netname.c
+++ b/sunrpc/netname.c
@@ -140,9 +140,6 @@  libc_hidden_nolink_sunrpc (getnetname, GLIBC_2_1)
 /* Type of the lookup function for netname2user.  */
 typedef int (*netname2user_function) (const char netname[MAXNETNAMELEN + 1],
 				      uid_t *, gid_t *, int *, gid_t *);
-/* The lookup function for the first entry of this service.  */
-extern int __nss_publickey_lookup (service_user ** nip, const char *name,
-				   void **fctp) internal_function;
 
 int
 netname2user (const char netname[MAXNETNAMELEN + 1], uid_t * uidp, gid_t * gidp,
@@ -161,7 +158,7 @@  netname2user (const char netname[MAXNETNAMELEN + 1], uid_t * uidp, gid_t * gidp,
 
   if (startp == NULL)
     {
-      no_more = __nss_publickey_lookup (&nip, "netname2user", &fct.ptr);
+      no_more = __nss_publickey_lookup2 (&nip, "netname2user", NULL, &fct.ptr);
       if (no_more)
 	startp = (service_user *) - 1;
       else
diff --git a/sunrpc/publickey.c b/sunrpc/publickey.c
index ca6e4303d4..e7ac8548f8 100644
--- a/sunrpc/publickey.c
+++ b/sunrpc/publickey.c
@@ -31,11 +31,6 @@  typedef int (*public_function) (const char *, char *, int *);
 /* Type of the lookup function for the secret key.  */
 typedef int (*secret_function) (const char *, char *, const char *, int *);
 
-/* The lookup function for the first entry of this service.  */
-extern int __nss_publickey_lookup (service_user **nip, const char *name,
-				   void **fctp) internal_function;
-
-
 int
 getpublickey (const char *name, char *key)
 {
@@ -52,7 +47,7 @@  getpublickey (const char *name, char *key)
 
   if (startp == NULL)
     {
-      no_more = __nss_publickey_lookup (&nip, "getpublickey", &fct.ptr);
+      no_more = __nss_publickey_lookup2 (&nip, "getpublickey", NULL, &fct.ptr);
       if (no_more)
 	startp = (service_user *) -1;
       else
@@ -95,7 +90,7 @@  getsecretkey (const char *name, char *key, const char *passwd)
 
   if (startp == NULL)
     {
-      no_more = __nss_publickey_lookup (&nip, "getsecretkey", &fct.ptr);
+      no_more = __nss_publickey_lookup2 (&nip, "getsecretkey", NULL, &fct.ptr);
       if (no_more)
 	startp = (service_user *) -1;
       else