Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

data_pool_* symbol visibility #160

Closed
paravoid opened this issue Dec 27, 2017 · 4 comments · Fixed by #162
Closed

data_pool_* symbol visibility #160

paravoid opened this issue Dec 27, 2017 · 4 comments · Fixed by #162

Comments

@paravoid
Copy link
Contributor

The new data_pool_* functions seem to be internal-only (or are at least undocumented?) but they appear on the library's exported symbols:

$ objdump -T libmaxminddb.so.0.0.7 | grep data_pool
0000000000003ce0 g    DF .text	000000000000007a  Base        data_pool_new
0000000000003e20 g    DF .text	0000000000000023  Base        data_pool_lookup
0000000000003cb0 g    DF .text	0000000000000022  Base        data_pool_destroy
0000000000003e50 g    DF .text	0000000000000054  Base        data_pool_to_list
0000000000003d60 g    DF .text	00000000000000b6  Base        data_pool_alloc

These can be hidden using gcc/ld tricks, like the visibility attribute or a version script with e.g. a MMDB_* pattern.

@paravoid
Copy link
Contributor Author

I've tried this:

--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -4,7 +4,7 @@ lib_LTLIBRARIES = libmaxminddb.la
 
 libmaxminddb_la_SOURCES = maxminddb.c maxminddb-compat-util.h \
        data-pool.c data-pool.h
-libmaxminddb_la_LDFLAGS = -version-info 0:7:0
+libmaxminddb_la_LDFLAGS = -version-info 0:7:0 -export-symbols-regex '^MMDB_.*'
 include_HEADERS = $(top_srcdir)/include/maxminddb.h
 
 pkgconfig_DATA = libmaxminddb.pc

…but then apparently the data pool test fails with:

data_pool_t-data-pool-t.o: In function `test_data_pool_new':
./t/data-pool-t.c:24: undefined reference to `data_pool_new'
./t/data-pool-t.c:29: undefined reference to `data_pool_new'
./t/data-pool-t.c:34: undefined reference to `data_pool_new'
./t/data-pool-t.c:39: undefined reference to `data_pool_destroy'
[etc.]

@horgh
Copy link
Contributor

horgh commented Dec 27, 2017

Hi @paravoid. Great catch! Yeah, they are intended to be internal.

Regarding the problem with the test, we could possibly do something like this:

--- a/t/Makefile.am
+++ b/t/Makefile.am
@@ -18,6 +18,7 @@ check_PROGRAMS = \

 data_pool_t_CFLAGS = $(CFLAGS) -I$(top_srcdir)/src
 data_pool_t_LDFLAGS = $(AM_LDFLAGS) -lm
+data_pool_t_SOURCES = data-pool-t.c $(top_builddir)/src/data-pool.c

 threads_t_CFLAGS = $(CFLAGS) -pthread

@paravoid
Copy link
Contributor Author

Thanks for the super quick response! That's a smart idea, I can confirm that the combination of the two patches above work as intended.

@horgh
Copy link
Contributor

horgh commented Dec 28, 2017

Cool, thanks for trying it!

As you did most of the work, would you want to make a PR with these changes? Otherwise I'll do so.

paravoid added a commit to paravoid/libmaxminddb that referenced this issue Dec 29, 2017
As to only export symbols starting with "MMDB_" and in lieu of a proper
version script. This helps by filtering out the new data_pool_* symbols,
which are internal-only and should not exposed to applications using
libmaxminddb.

The change to LDFLAGS would introduce a test failure, as data-pool-t
used those internal functions, but this is addressed by adding
src/data-pool.c to data-pool-t's SOURCES, as suggested by Will Storey.

This resolves maxmind#160.
paravoid added a commit to paravoid/libmaxminddb that referenced this issue Dec 31, 2017
As to only export symbols starting with "MMDB_" and in lieu of a proper
version script. This helps by filtering out the new data_pool_* symbols,
which are internal-only and should not exposed to applications using
libmaxminddb.

The change to LDFLAGS would introduce a test failure, as data-pool-t
used those internal functions, but this is addressed by adding
src/data-pool.c to data-pool-t's SOURCES, as suggested by Will Storey.

This resolves maxmind#160.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants