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

Jakub Hrozek jhrozek at redhat.com
Fri Sep 6 14:06:07 UTC 2013


On Fri, Sep 06, 2013 at 09:02:27AM -0400, Simo Sorce wrote:
> 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.
> 


That's fine, adding a new provider is not a common operation after all.

Maybe we could add a comment to the Makefile.am instructing the
developer to watch out for this test when adding a new library?

> 
> > > +
> > > +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.

ACK from me. I'll wait until Lukas finishes his investigation before
pushing.

btw the error I was seeing wasn't about parallel build per se but about
my setup having different source dir, build dir and test dir..the cwd
would change the working directory to test dir and then attempt to load
objects from build dir.



More information about the sssd-devel mailing list