Bug 246596 - Memory leak in ctld
Summary: Memory leak in ctld
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Mark Johnston
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-20 09:40 UTC by Patryk
Modified: 2020-06-03 14:55 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Patryk 2020-05-20 09:40:11 UTC
In ucl_parse.c file there is memory leak:

int
uclparse_conf(struct conf *newconf, const char *path)
{
	struct ucl_parser *parser;
	int error; 

	conf = newconf;
	parser = ucl_parser_new(0);

	if (!ucl_parser_add_file(parser, path)) {
		log_warn("unable to parse configuration file %s: %s", path,
		    ucl_parser_get_error(parser));
		return (1);
	}

	error = uclparse_toplevel(ucl_parser_get_object(parser));

	return (error);
}

ucl_parser pointer is never freed. 

My fix proposal is following (I tested it in my production environment):


int
uclparse_conf(struct conf *newconf, const char *path)
{
	struct ucl_parser *parser;
	int error; 

	conf = newconf;
	parser = ucl_parser_new(0);

	if (!ucl_parser_add_file(parser, path)) {
		log_warn("unable to parse configuration file %s: %s", path,
		    ucl_parser_get_error(parser));
		return (1);
	}

	ucl_object_t *p_top = ucl_parser_get_object(parser);
	error = uclparse_toplevel(p_top);

	ucl_object_unref(p_top);
	ucl_parser_free(parser);

	return (error);
}
Comment 1 Patryk 2020-05-20 10:15:46 UTC
Also I found another possible memory leak. In ctld.c in conf_apply fucntion there is:

	/*
	 * Second, remove any LUNs present in the old configuration
	 * and missing in the new one.
	 */
	TAILQ_FOREACH_SAFE(oldlun, &oldconf->conf_luns, l_next, tmplun) {
		newlun = lun_find(newconf, oldlun->l_name);
		if (newlun == NULL) {
			log_debugx("lun \"%s\", CTL lun %d "
			    "not found in new configuration; "
			    "removing", oldlun->l_name, oldlun->l_ctl_lun);
			error = kernel_lun_remove(oldlun);
			if (error != 0) {
				log_warnx("failed to remove lun \"%s\", "
				    "CTL lun %d",
				    oldlun->l_name, oldlun->l_ctl_lun);
				cumulated_error++;
			}
			continue;
		}

Why here lun_delete function is not called before "continue;"?

I next part of function where luns with changed size are deleted there is lun_delete() call: 

		if (changed) {
			error = kernel_lun_remove(oldlun);
			if (error != 0) {
				log_warnx("failed to remove lun \"%s\", "
				    "CTL lun %d",
				    oldlun->l_name, oldlun->l_ctl_lun);
				cumulated_error++;
			}
			lun_delete(oldlun);
			continue;
		}

I did't tested in but I think this is also memory leak.
Comment 2 Mark Johnston freebsd_committer 2020-05-30 19:03:27 UTC
(In reply to Patryk from comment #1)
I suspect there is a similar leak when kernel_port_remove() fails.
Comment 3 commit-hook freebsd_committer 2020-05-30 19:12:58 UTC
A commit references this bug:

Author: markj
Date: Sat May 30 19:11:41 UTC 2020
New revision: 361654
URL: https://svnweb.freebsd.org/changeset/base/361654

Log:
  ctld: Fix a memory leak in uclparse_conf().

  PR:		246596
  Submitted by:	Patryk <patrykkotlowski@gmail.com>
  MFC after:	1 week

Changes:
  head/usr.sbin/ctld/uclparse.c
Comment 4 Mark Johnston freebsd_committer 2020-05-30 19:17:14 UTC
I submitted https://reviews.freebsd.org/D25074 for the second bug.
Comment 5 Mark Johnston freebsd_committer 2020-05-31 18:56:48 UTC
(In reply to Mark Johnston from comment #4)
mav@ points out that conf_delete() will handle this, so I think only the parser code change is required.
Comment 6 Patryk 2020-06-03 07:24:24 UTC
(In reply to Mark Johnston from comment #5)
Sounds great to me. 
Thank you :) 

Patryk
Comment 7 commit-hook freebsd_committer 2020-06-03 14:55:23 UTC
A commit references this bug:

Author: markj
Date: Wed Jun  3 14:54:55 UTC 2020
New revision: 361753
URL: https://svnweb.freebsd.org/changeset/base/361753

Log:
  MFC r361654:
  ctld: Fix a memory leak in uclparse_conf().

  PR:	246596

Changes:
_U  stable/12/
  stable/12/usr.sbin/ctld/uclparse.c
Comment 8 Mark Johnston freebsd_committer 2020-06-03 14:55:45 UTC
Thanks for the report.