Segfault in combination with reading files followed by config_clear()

[jin-eld]

Hi,

I am looking at a segfault and as far as I can tell I am not doing anything wrong...

The trigger seems to be a call to config_clear() combined with loading of a configuration file. If I comment config_clear() out, then it won't crash. The actual segfault happens in config_destroy().

Sample config (save as /tmp/test.cfg):

somevalue = 0
somegroup = 
{
  address = "something.com"
  port = 45000
}

Code:

#include <libconfig.h>
#include <stdio.h>
#include <stdlib.h>

int main()
{
    config_t cfg_current;
    config_init(&cfg_current);
    config_set_options(&cfg_current, CONFIG_OPTION_OPEN_BRACE_ON_SEPARATE_LINE |
                                         CONFIG_OPTION_FSYNC);

    config_setting_t *root = NULL;
    config_setting_t *setting = NULL;

    if (config_read_file(&cfg_current, "/tmp/test.cfg") != CONFIG_TRUE)
    {
        config_destroy(&cfg_current);
        return 1;
    }

    /* if you comment out the call to config_clear(), it won't crash! */
    config_clear(&cfg_current);

    root = config_root_setting(&cfg_current);
    if (root)
    {
        setting = config_setting_add(root, "test", CONFIG_TYPE_INT);
        if (setting)
        {
            config_setting_set_int(setting, 123);
        }
    }

    config_write_file(&cfg_current, "/tmp/new.cfg");

    config_destroy(&cfg_current); /* This is line 35 in the backtrace, the crash is happening in config_destroy() */

    return 0;
}

To compile: gcc -ggdb -O0 -o test test.c $(pkg-config --cflags --libs libconfig)

Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7e4ef00 in free () from /lib64/libc.so.6
(gdb) bt
#0  0x00007ffff7e4ef00 in free () from /lib64/libc.so.6
#1  0x00007ffff7f92ec5 in __delete_vec (v=<optimized out>) at util.c:40
#2  __delete_vec (v=0x405540) at util.c:33
#3  0x00007ffff7f92f05 in config_destroy (config=0x7fffffffe070)
    at libconfig.c:721
#4  0x000000000040125e in main () at test.c:35

The saved file /tmp/new.cfg looks as expected in both cases, i.e. with and without the call to config_clear().

Am I using the library in a wrong way, or is this a bug?

[hyperrealm]

Looks like you're using a pretty old version of the library, because that stack trace doesn't match up to the code.

[jin-eld]

Oops, I should have posted the version of course: libconfig-1.7.2-6.fc33.x86_64 which is the latest offered by Fedora 33, 1.7.2 seems also to be the latest release version available on https://hyperrealm.github.io/libconfig/

[hyperrealm]

Looks like I've been lazy with the releases :-( I'll make a new one within the next couple of days.

[jin-eld]

Cool, thank you! I'll retest as soon as it's out :>

[jin-eld]

I wanted to check if the problem got fixed in the newer code I pulled master HEAD (commit 5ea446216255e90bfdbf646a06404e37651f192b) and retried, seems that the issue is still there:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7e4ef00 in free () from /lib64/libc.so.6
(gdb) bt
#0  0x00007ffff7e4ef00 in free () from /lib64/libc.so.6
#1  0x00007ffff7fc3b85 in libconfig_strvec_delete (vec=0x405540) at strvec.c:69
#2  0x00007ffff7fc0895 in config_destroy (config=0x7fffffffdff0)
    at libconfig.c:723
#3  0x000000000040125e in main () at test.c:36

[jin-eld]

I suspect the issue is that config_destroy() tries to free config->filenames which has already been freed by config_clear().

A quick fix that works for me is to set config->filenames back to NULL after freeing:

diff --git a/lib/libconfig.c b/lib/libconfig.c
index e26e5e8..38e8623 100644
--- a/lib/libconfig.c
+++ b/lib/libconfig.c
@@ -732,7 +732,7 @@ void config_clear(config_t *config)
   /* Destroy the root setting (recursively) and then create a new one. */
   __config_setting_destroy(config->root);
   libconfig_strvec_delete(config->filenames);
-
+  config->filenames = NULL;
   config->root = __new(config_setting_t);
   config->root->type = CONFIG_TYPE_GROUP;
   config->root->config = config;

[thomastrapp]

@jin-eld Your fix looks good to me.

If config->filenames is meant to be reused, it has to be set to NULL after freeing memory.

The unit tests do not fail and Valgrind does not complain.

[jin-eld]

@thomastrapp Thanks for checking, I was not sure if it had any deeper implications as I did not have time to dig into other functions that make use of config->filenames, the unit tests did not fail indeed.

@hyperrealm if you'd like me to submit a PR, I can do that, I felt its probably not worth for a 1-liner and easier for you to quick patch yourself. Looking forward to a new release with the fix :)

[jin-eld]

@hyperrealm any chance you could roll a new release?

[hyperrealm]

Sorry for delay...this turned out to be a double-free of config->filenames, because it wasn't set to NULL in config_clear(). I'll have a new release with this fix shortly.

[hyperrealm]

This is fixed in the new release 1.7.3

[jin-eld]

Great, thank you!

[hyperrealm]

Sure thing. I just now noticed the earlier comments where you'd found the solution yourself!