Memory leak in _load_default_configs()

Question

Memory leak in _load_default_configs()

lcatro opened this issue 7 years ago · 6 comments

ASAN Detect :

The reason is load_config() return json_error by malloc() ,but _load_default_configs() never free this return .

Fix :

static
bool _load_default_configs(const char * const filepath, void * __maybe_unused userp)
{
        bool * const found_defcfg_p = userp;
        *found_defcfg_p = true;

        char* buffer = load_config(filepath, NULL);

        if (NULL != buffer)
                free(buffer);

        // Regardless of status of loading the config file, we should continue loading other defaults
        return false;
}

Answer 1 · 2017-12-20T04:59:18.000Z

Pretty minor since it is just a small leak during startup in an error condition.

Unfortunately, fixing it isn't that simple: you can't free the other return values, and ccan opt needs to handle the return value properly as well.

Answer 2 · 2017-12-20T05:02:43.000Z

@luke-jr Oh ,I'm forget something .parse_config() will return a buffer ,and this is a static buffer .I change this buffer alloc by malloc() .

Answer 3 · 2017-12-20T05:07:08.000Z

@luke-jr

this is my fix code at parse_config()

static char *parse_config(json_t *config, bool fileconf, int * const fileconf_load_p)
{
        //static char err_buf[200];
        struct opt_table *opt;
        json_t *val;

        if (fileconf && !*fileconf_load_p)
                *fileconf_load_p = 1;

        for (opt = opt_config_table; opt->type != OPT_END; opt++) {
                char *p, *name, *sp;

                /* We don't handle subtables. */
                assert(!(opt->type & OPT_SUBTABLE));

                if (!opt->names)
                        continue;

                /* Pull apart the option name(s). */
                name = strdup(opt->names);
                for (p = strtok_r(name, "|", &sp); p; p = strtok_r(NULL, "|", &sp)) {
                        char *err = "Invalid value";

                        /* Ignore short options. */
                        if (p[1] != '-')
                                continue;

                        val = json_object_get(config, p+2);
                        if (!val)
                                continue;

                        if (opt->type & OPT_HASARG) {
                          if (json_is_string(val)) {
                                err = opt->cb_arg(json_string_value(val),
                                                  opt->u.arg);
                          } else if (json_is_number(val)) {
                                        char buf[256], *p, *q;
                                        snprintf(buf, 256, "%f", json_number_value(val));
                                        if ( (p = strchr(buf, '.')) ) {
                                                // Trim /\.0*$/ to work properly with integer-only arguments
                                                q = p;
                                                while (*(++q) == '0') {}
                                                if (*q == '\0')
                                                        *p = '\0';
                                        }
                                        err = opt->cb_arg(buf, opt->u.arg);
                          } else if (json_is_array(val)) {
                                int n, size = json_array_size(val);

                                err = NULL;
                                for (n = 0; n < size && !err; n++) {
                                        if (json_is_string(json_array_get(val, n)))
                                                err = opt->cb_arg(json_string_value(json_array_get(val, n)), opt->u.arg);
                                        else if (json_is_object(json_array_get(val, n)))
                                                err = parse_config(json_array_get(val, n), false, fileconf_load_p);
                                }
                          }
                        } else if (opt->type & OPT_NOARG) {
                                if (json_is_true(val))
                                        err = opt->cb(opt->u.arg);
                                else if (json_is_boolean(val)) {
                                        if (opt->cb == (void*)opt_set_bool)
                                                err = opt_set_invbool(opt->u.arg);
                                        else if (opt->cb == (void*)opt_set_invbool)
                                                err = opt_set_bool(opt->u.arg);
                                }
                        }

                        if (err) {
                                /* Allow invalid values to be in configuration
                                 * file, just skipping over them provided the
                                 * JSON is still valid after that. */
                                if (fileconf) {
                                        applog(LOG_ERR, "Invalid config option %s: %s", p, err);
                                        *fileconf_load_p = -1;
                                } else {
                                        char* err_buf = malloc(200);
                                        snprintf(err_buf, sizeof(err_buf), "Parsing JSON option %s: %s",
                                                p, err);
                                        free(name);
                                        return err_buf;
                                }
                        }
                }
                free(name);
        }

        val = json_object_get(config, JSON_INCLUDE_CONF);
        if (val && json_is_string(val))
                return load_config(json_string_value(val), NULL);

        return NULL;
}

Answer 4 · 2017-12-20T05:09:24.000Z

This would be easier to review as a pull request...

Answer 5 · 2017-12-20T05:20:42.000Z

@luke-jr I'm try to fuzzing bfgminer and fix more code at last mouth .now forget these codes where i rewrite .So sorry . :)