[SSSD] [PATCH] Check our libraries properly resolve symbols

Simo Sorce simo at redhat.com
Fri Sep 6 13:02:27 UTC 2013


On Fri, 2013-09-06 at 13:50 +0200, Jakub Hrozek wrote:
> On Thu, Sep 05, 2013 at 09:48:27PM -0400, Simo Sorce wrote:
> > While reviewing the recent big patchset I sent Sumit found out I did not
> > add some .c files to the ldap provider (which I didn't test as I was
> > working on the krb5 provider), and that broke the provider's shared
> > library as one symbols was missing.
> > 
> > We allow modules to have unresolved symbols because a number of the
> > symbols they use come from our sssd_be binary so they can't be fully
> > resolved at build time, only at runtime.
> > 
> > The attached patch adds a test that simulates loading most of the shared
> > modules we build in order to verify that there are no unresolved
> > symbols, that is done by manually dlopen()ing eash .so library with
> > RTLD_NOW so all symbols are immediately resolved.
> > 
> > I tested that this test properly caught the issue Sumit found before
> > fixing it.
> > 
> > Simo.
> 
> Hi,
> 
> I think in general this is a great test to have, thank you. See some
> minor comments below:
> 
> > 
> > -- 
> > Simo Sorce * Red Hat, Inc * New York
> 
> > From 53d8be3d2b543fec3aa83a7cfa86dcd87c7425d7 Mon Sep 17 00:00:00 2001
> > From: Simo Sorce <simo at redhat.com>
> > Date: Thu, 5 Sep 2013 11:52:08 -0400
> > Subject: [PATCH] tests: Add dlopen test to make sure modules works
> > 
> > This tests dlopens and resolves all symbols to make sure there are no missing
> > symbols in our provider modules.
> > ---
> >  Makefile.am              |  23 ++++++++++
> >  src/tests/dlopen-tests.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 135 insertions(+)
> >  create mode 100644 src/tests/dlopen-tests.c
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 05da9a14d82d4d0c11ec2eb888e6af46c6e48ff7..6aad7530120e783c9a1c937110b22b49cf6139eb 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -114,6 +114,7 @@ endif
> >  
> >  if HAVE_CHECK
> >      non_interactive_check_based_tests = \
> > +	dlopen-tests \
> 
> Mixed tab and space

Fixed.

> >          sysdb-tests \
> >          strtonum-tests \
> >          resolv-tests \
> 
> > index 0000000000000000000000000000000000000000..4067162eb63feb0ebdf21b250f83bdfcb4654182
> > --- /dev/null
> > +++ b/src/tests/dlopen-tests.c
> > @@ -0,0 +1,112 @@
> > +/*
> > +    SSSD
> > +
> > +    debug-tests.c
> > +
> > +    Authors:
> > +        Pavel Březina <pbrezina at redhat.com>
> 
> You should credit yourself :)


Indeed


> > +
> > +    Copyright (C) 2011 Red Hat


And changed year too ^^


> > +    This program is free software; you can redistribute it and/or modify
> > +    it under the terms of the GNU General Public License as published by
> > +    the Free Software Foundation; either version 3 of the License, or
> > +    (at your option) any later version.
> > +
> > +    This program 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 General Public License for more details.
> > +
> > +    You should have received a copy of the GNU General Public License
> > +    along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +*/
> > +
> > +#define _GNU_SOURCE
> > +#include <stdbool.h>
> > +#include <dlfcn.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <limits.h>
> > +#include <check.h>
> > +#include "tests/common.h"
> > +
> > +#define LIBPFX DLOPEN_TESTS_BUILDDIR"/.libs/"
> 
> We talked about this hardcoded directory on IRC and I can't see another
> way to autodetect this either.


So far no good way indeed, the joys of libtool ...


> > +
> > +const char *sonames[] = {
> > +    "libipa_hbac.so",
> > +    "libsss_ad.so",
> > +    "libsss_child.so",
> > +    "libsss_debug.so",
> > +    "libsss_ipa.so",
> > +    "libsss_krb5.so",
> > +    "libsss_ldap.so",
> > +    "libsss_proxy.so",
> > +    "libsss_sudo.so",
> > +    "memberof.so",
> > +    "pyhbac.so",
> > +    "pysss_nss_idmap.so",
> > +    "sssd_krb5_locator_plugin.so",
> > +    "libnss_sss.so",
> > +    "libsss_autofs.so",
> > +    "libsss_crypt.so",
> > +    "libsss_idmap.so",
> > +    "libsss_nss_idmap.so",
> > +    "libsss_simple.so",
> > +    "libsss_util.so",
> > +    "pam_sss.so",
> > +    "sssd_pac_plugin.so",
> > +    NULL
> > +};
> 
> I wonder if there's any way of autogenerating this maybe from
> LTLIBRARIES? But definitely not something worth fixing in this patch
> instance.


Not sure, if you look at the list carefully you'll see 2 .so objects are
missing (sss_krb5_common.so and sss_ldap_common.so) because they would
fail the test on their own and they are already implicitly tested by
provider modules which have these libraries as dependencies (see output
of ldd .libs/libss_ldap.so for example).

I actually initially wanted to restrict to only the providers librraries
given all others are built with dependency checking, but given it cost
nothing to test them all and I used a sed command to go from ls -> list
and it all worked I left most of them in there.


> > +
> > +START_TEST(test_dlopen_base)
> > +{
> > +    void *handle;
> > +    char *soname;
> > +    int ret, i;
> > +
> > +    for (i = 0; sonames[i] != NULL; i++) {
> > +        ret = asprintf(&soname, LIBPFX"%s", sonames[i]);
> > +        fail_unless(ret != -1, "Failed to construct soname");
> > +
> > +        handle = dlopen(soname, RTLD_NOW);
> > +        fail_unless(handle != NULL,
> > +                    "dlopen() failed for %s: [%s]", soname, dlerror());
> > +        dlclose(handle);
> > +        free(soname);
> > +    }
> > +}
> > +END_TEST
> > +
> > +Suite *dlopen_suite(void)
> > +{
> > +    Suite *s = suite_create("dlopen");
> > +
> > +    TCase *tc_dlopen = tcase_create("dlopen");
> > +
> > +    tcase_add_test(tc_dlopen, test_dlopen_base);
> > +    tcase_set_timeout(tc_dlopen, 10);
> > +
> > +    suite_add_tcase(s, tc_dlopen);
> > +
> > +    return s;
> > +}
> > +
> > +int main(int argc, const char *argv[])
> > +{
> > +    int number_failed;
> > +
> > +    tests_set_cwd();
> > +
> 
> I think this test is a special case where we actually should not call
> tests_set_cwd(). Calling it breaks the parallel build for me.


Removed.


> > +    Suite *s = dlopen_suite();
> > +    SRunner *sr = srunner_create(s);
> > +
> > +    srunner_run_all(sr, CK_NORMAL);
> > +    number_failed = srunner_ntests_failed(sr);
> > +    srunner_free(sr);
> > +
> > +    if (number_failed == 0)
> > +        return EXIT_SUCCESS;
> > +
> > +    return EXIT_FAILURE;
> > +}
> > -- 
> > 1.8.3.1
> > 
> 
> Otherwise looks good to me!


Ok, fixed patch attached.

Simo.


-- 
Simo Sorce * Red Hat, Inc * New York
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-tests-Add-dlopen-test-to-make-sure-modules-works.patch
Type: text/x-patch
Size: 4569 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130906/75110a91/attachment.bin>


More information about the sssd-devel mailing list