Dynamic loading of journald symbols fails

[HofiOne]

syslog-ng

Version of syslog-ng

syslog-ng 4 (4.0.1.96.g1635664)

Platform

Ubuntu 22.04.1 LTS
debian-buster_amd64
oracle-7_amd64
sles-12_amd64
ubuntu-xenial_amd64
rhel-7_amd64

Issue

syslog-ng crashes during loading of module='sdjournal'

Failure

[2023-02-01T14:08:20.879032] Module loaded and initialized successfully; module='system-source'
[2023-02-01T14:08:20.879074] Trying to open module; module='sdjournal', filename='/home/test/Develop/syslog-ng/build/install/lib/syslog-ng/libsdjournal.so'

Program received signal SIGSEGV, Segmentation fault.
0x0000fffff7a9162c in g_module_symbol () from /lib/aarch64-linux-gnu/libgmodule-2.0.so.0
(gdb) bt
#0 0x0000fffff7a9162c in g_module_symbol () from /lib/aarch64-linux-gnu/libgmodule-2.0.so.0
#1 0x0000fffff708a6d4 in _load_journald_symbols () at ../modules/systemd-journal/journald-subsystem.c:79
#2 0x0000fffff708aa80 in load_journald_subsystem () at ../modules/systemd-journal/journald-subsystem.c:145
#3 0x0000fffff70879cc in systemd_journal_module_init (context=0xaaaaaaad5a40, args=0x0) at ../modules/systemd-journal/systemd-journal-plugin.c:44
#4 0x0000fffff7ecc8b0 in plugin_load_module (context=0xaaaaaaad5a40, module_name=0xaaaaaaae9e80 "sdjournal", args=0x0) at ../lib/plugin.c:405
#5 0x0000fffff7ecc628 in plugin_find (context=0xaaaaaaad5a40, plugin_type=3, plugin_name=0xaaaaaac1e630 "systemd-journal") at ../lib/plugin.c:337
#6 0x0000fffff7e9d904 in cfg_find_plugin (cfg=0xaaaaaaad5a30, plugin_type=3, plugin_name=0xaaaaaac1e630 "systemd-journal") at ../lib/cfg.c:239
#7 0x0000fffff7ede20c in main_parse (lexer=0xaaaaaabb71b0, dummy=0xffffffffedb8, arg=0x0) at ../lib/cfg-grammar.y:547
#8 0x0000fffff7ea40fc in cfg_parser_parse (self=0xfffff7fab020 <main_parser>, lexer=0xaaaaaabb71b0, instance=0xffffffffedb8, arg=0x0) at ../lib/cfg-parser.c:408
#9 0x0000fffff7e9e628 in cfg_run_parser (self=0xaaaaaaad5a30, lexer=0xaaaaaabb71b0, parser=0xfffff7fab020 <main_parser>, result=0xffffffffedb8, arg=0x0) at ../lib/cfg.c:596
#10 0x0000fffff7e9e94c in cfg_read_config (self=0xaaaaaaad5a30, fname=0xaaaaaaabcac0 "/home/test/Develop/syslog-ng/build/install/etc/syslog-ng.conf", preprocess_into=0x0) at ../lib/cfg.c:683
#11 0x0000fffff7ec2990 in main_loop_read_and_init_config (self=0xfffff7fae320 <main_loop>) at ../lib/mainloop.c:618
#12 0x0000aaaaaaaa263c in main (argc=1, argv=0xffffffffefd8) at ../syslog-ng/main.c:307

Steps to reproduce

configure and build syslog-ng with --with-systemd-journal=optional switch than start it with the default configuration

Configuration

default (that uses system journald)

By a good chance this issue is related to changes of #4245

[bazsi]

:( I am not sure that code is worth keeping, I've tried to keep the optional mode of operation in the codebase but I had an itch to remove it. @HofiOne we tried to decipher why/where this is used, but do you have an idea?

In the meanwhile I am trying to reproduce this one. And maybe we should add something that checks this into the CI.

[bazsi]

The issue is that even though we are using --with-systemd-journal=optional we are still linking against libsystemd.so through libsyslog-ng.so and we have two instances of the same name: at one time it is a global variable (pointer) another time it is a function with an executable code.

The pointer we pass to g_module_symbol() to store the address to points to the executable function and obviously that's a read only area, causing g_module_symbol() to crash.

I wouldn't want to decipher the reasons how the linker is able to map an imported symbol over a variable that is local to the compile unit, but I guess the easiest way forward is to rename those damn variables to have a different name...

[bazsi]

Something like this fixes it without having to rename/remap those variables:

diff --git a/modules/systemd-journal/journald-subsystem.h b/modules/systemd-journal/journald-subsystem.h
index 7987e74df..2ae6db246 100644
--- a/modules/systemd-journal/journald-subsystem.h
+++ b/modules/systemd-journal/journald-subsystem.h
@@ -65,25 +65,25 @@ union sd_id128
 
 typedef struct sd_journal sd_journal;
 
-extern int (*sd_journal_open)(sd_journal **ret, int flags);
+extern int (*sd_journal_open)(sd_journal **ret, int flags) __attribute__((visibility("hidden")));
 #if SYSLOG_NG_HAVE_JOURNAL_NAMESPACES
-extern int (*sd_journal_open_namespace)(sd_journal **ret, const char *namespace, int flags);
+extern int (*sd_journal_open_namespace)(sd_journal **ret, const char *namespace, int flags) __attribute__((visibility("hidden")));
 #endif
-extern void (*sd_journal_close)(sd_journal *j);
-extern int (*sd_journal_seek_head)(sd_journal *j);
-extern int (*sd_journal_seek_tail)(sd_journal *j);
-extern int (*sd_journal_get_cursor)(sd_journal *j, char **cursor);
-extern int (*sd_journal_next)(sd_journal *j);
-extern void (*sd_journal_restart_data)(sd_journal *j);
-extern int (*sd_journal_enumerate_data)(sd_journal *j, const void **data, size_t *length);
-extern int (*sd_journal_seek_cursor)(sd_journal *j, const char *cursor);
-extern int (*sd_journal_test_cursor)(sd_journal *j, const char *cursor);
-extern int (*sd_journal_get_fd)(sd_journal *j);
-extern int (*sd_journal_process)(sd_journal *j);
-extern int (*sd_journal_get_realtime_usec)(sd_journal *j, uint64_t *usec);
-extern int (*sd_journal_add_match)(sd_journal *j, const void *data, size_t size);
-extern char *(*sd_id128_to_string)(sd_id128_t id, char s[SD_ID128_STRING_MAX]);
-extern int (*sd_id128_get_boot)(sd_id128_t *ret);
+extern void (*sd_journal_close)(sd_journal *j) __attribute__((visibility("hidden")));
+extern int (*sd_journal_seek_head)(sd_journal *j) __attribute__((visibility("hidden")));
+extern int (*sd_journal_seek_tail)(sd_journal *j) __attribute__((visibility("hidden")));
+extern int (*sd_journal_get_cursor)(sd_journal *j, char **cursor) __attribute__((visibility("hidden")));
+extern int (*sd_journal_next)(sd_journal *j) __attribute__((visibility("hidden")));
+extern void (*sd_journal_restart_data)(sd_journal *j) __attribute__((visibility("hidden")));
+extern int (*sd_journal_enumerate_data)(sd_journal *j, const void **data, size_t *length) __attribute__((visibility("hidden")));
+extern int (*sd_journal_seek_cursor)(sd_journal *j, const char *cursor) __attribute__((visibility("hidden")));
+extern int (*sd_journal_test_cursor)(sd_journal *j, const char *cursor) __attribute__((visibility("hidden")));
+extern int (*sd_journal_get_fd)(sd_journal *j) __attribute__((visibility("hidden")));
+extern int (*sd_journal_process)(sd_journal *j) __attribute__((visibility("hidden")));
+extern int (*sd_journal_get_realtime_usec)(sd_journal *j, uint64_t *usec) __attribute__((visibility("hidden")));
+extern int (*sd_journal_add_match)(sd_journal *j, const void *data, size_t size) __attribute__((visibility("hidden")));
+extern char *(*sd_id128_to_string)(sd_id128_t id, char s[SD_ID128_STRING_MAX]) __attribute__((visibility("hidden")));
+extern int (*sd_id128_get_boot)(sd_id128_t *ret) __attribute__((visibility("hidden")));
 
 int load_journald_subsystem(void);
 

[HofiOne]

The issue is that even though we are using --with-systemd-journal=optional we are still linking against libsystemd.so through libsyslog-ng.so and we have two instances of the same name: at one time it is a global variable (pointer) another time it is a function with an executable code.

The pointer we pass to g_module_symbol() to store the address to points to the executable function and obviously that's a read only area, causing g_module_symbol() to crash.

I wouldn't want to decipher the reasons how the linker is able to map an imported symbol over a variable that is local to the compile unit, but I guess the easiest way forward is to rename those damn variables to have a different name...

more or less it was the same I figured out, I tried to find a solution that will force g_module_symbole() to ignore our version of the given symbol, but you were quicker, I've never met with __attribute((visibility("hidden"))) before
just curious, is that supported by all the compilers we want to build syslog-ng?

[bazsi]

gcc and clang supports it and it can always be ifdefed if needed. We have a couple of those attributes in our codebase already, so I wouldn't worry too much about that.

[HofiOne]

centos still has a similar issue, as we discussed I'll add support for it too