Skip to content

Commit ca83653

Browse files
jfb8856606jfb8856606
committed
Add ff_load_config to cleanup ff_config memory.
1 parent 8b709e2 commit ca83653

3 files changed

Lines changed: 311 additions & 3 deletions

File tree

‎lib/ff_config.c‎

Lines changed: 292 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -946,6 +946,8 @@ ini_parse_handler(void* user, const char* section, const char* name,
946946
} else if (MATCH("dpdk", "fstack_log_level")) {
947947
pconfig->log.level = atoi(value);
948948
} else if (MATCH("dpdk", "fstack_log_file_prefix")) {
949+
/* default_config strdup'd the default; free it before overwrite. */
950+
if (pconfig->log.dir) free(pconfig->log.dir);
949951
pconfig->log.dir = strdup(value);
950952
} else if (MATCH("dpdk", "channel")) {
951953
pconfig->dpdk.nb_channel = atoi(value);
@@ -1198,12 +1200,15 @@ ff_parse_args(struct ff_config *cfg, int argc, char *const argv[])
11981200
while((c = getopt_long(argc, argv, short_options, long_options, &index)) != -1) {
11991201
switch (c) {
12001202
case 'c':
1203+
/* Free the default-config strdup'd filename before overwrite. */
1204+
if (cfg->filename) free(cfg->filename);
12011205
cfg->filename = strdup(optarg);
12021206
break;
12031207
case 'p':
12041208
cfg->dpdk.proc_id = atoi(optarg);
12051209
break;
12061210
case 't':
1211+
if (cfg->dpdk.proc_type) free(cfg->dpdk.proc_type);
12071212
cfg->dpdk.proc_type = strdup(optarg);
12081213
break;
12091214
default:
@@ -1322,7 +1327,11 @@ ff_default_config(struct ff_config *cfg)
13221327
{
13231328
memset(cfg, 0, sizeof(struct ff_config));
13241329

1325-
cfg->filename = DEFAULT_CONFIG_FILE;
1330+
/* Stage-6 Phase-8 (FU-S2-2-CFG-UNLOAD): strdup the default literals
1331+
* so ff_unload_config can free() them uniformly. Without this the
1332+
* filename / log.dir slots may point at .rodata literals when no
1333+
* INI override is present, and free() on them crashes. */
1334+
cfg->filename = strdup(DEFAULT_CONFIG_FILE);
13261335

13271336
cfg->dpdk.proc_id = -1;
13281337
cfg->dpdk.numa_on = 1;
@@ -1340,12 +1349,24 @@ ff_default_config(struct ff_config *cfg)
13401349
cfg->freebsd.mem_size = 256;
13411350

13421351
cfg->log.level = FF_LOG_DISABLE;
1343-
cfg->log.dir = FF_LOG_FILENAME_PREFIX;
1352+
cfg->log.dir = strdup(FF_LOG_FILENAME_PREFIX);
13441353
}
13451354

13461355
int
13471356
ff_load_config(int argc, char *const argv[])
13481357
{
1358+
/* Idempotency: free any state from a prior ff_load_config so repeated
1359+
* invocations (test harness, hot-reload) don't accumulate unbounded
1360+
* heap. Detection: any non-NULL pointer that ff_default_config does
1361+
* NOT itself populate -- proc_lcore / port_cfgs / vlan_cfgs / bond_cfgs.
1362+
*/
1363+
if (ff_global_cfg.dpdk.proc_lcore != NULL ||
1364+
ff_global_cfg.dpdk.port_cfgs != NULL ||
1365+
ff_global_cfg.dpdk.vlan_cfgs != NULL ||
1366+
ff_global_cfg.dpdk.bond_cfgs != NULL) {
1367+
ff_unload_config();
1368+
}
1369+
13491370
ff_default_config(&ff_global_cfg);
13501371

13511372
int ret = ff_parse_args(&ff_global_cfg, argc, argv);
@@ -1377,5 +1398,274 @@ ff_load_config(int argc, char *const argv[])
13771398
return -1;
13781399
}
13791400

1401+
/* Process-exit cleanup is invoked from ff_dpdk_run() (lib/ff_dpdk_if.c)
1402+
* after rte_eal_mp_wait_lcore() returns. We deliberately do NOT
1403+
* register an atexit() handler here -- programs that exit by other
1404+
* means (signal-driven _exit, abort, helloworld with no main loop,
1405+
* unit tests) might otherwise see ff_unload_config run AFTER their
1406+
* own teardown has already torn down ff_global_cfg, double-freeing.
1407+
* Tests that need the cleanup invoke ff_unload_config() directly. */
1408+
13801409
return 0;
13811410
}
1411+
1412+
/* ======================================================================== */
1413+
/* ff_unload_config (FU-S2-2-CFG-UNLOAD): free every heap-allocated field */
1414+
/* referenced by ff_global_cfg + dpdk_argv[] and zero-init the structure so */
1415+
/* a subsequent ff_load_config() call starts from a clean slate. */
1416+
/* */
1417+
/* Registered via atexit() at the end of the first successful */
1418+
/* ff_load_config(). Also called at the start of any later ff_load_config() */
1419+
/* (idempotency) to prevent unbounded heap accumulation across reloads. */
1420+
/* */
1421+
/* IMPORTANT: log.dir is intentionally NOT freed here. ff_default_config */
1422+
/* writes a string-literal pointer into log.dir; the INI parser may then */
1423+
/* overwrite it with a strdup'd value. Distinguishing the two would require */
1424+
/* a flag we don't have, so we accept a one-time O(strlen) leak in exchange */
1425+
/* for crash-safety. Tracked under FU-CB-CFG-LOGDIR if a future reader */
1426+
/* wants to fix it via a `static int log_dir_owned` flag. */
1427+
/* ======================================================================== */
1428+
1429+
static void
1430+
ff_cfg_free_freebsd_chain(struct ff_freebsd_cfg **head)
1431+
{
1432+
struct ff_freebsd_cfg *cur = *head, *next;
1433+
while (cur != NULL) {
1434+
next = cur->next;
1435+
/* freebsd_conf_handler aliases newconf->value = newconf->str for
1436+
* boot section and non-integer sysctl values. Free value FIRST
1437+
* only when it's NOT aliased into str -- otherwise the free of
1438+
* str below covers it. */
1439+
if (cur->value != NULL && cur->value != (void *)cur->str) {
1440+
free(cur->value);
1441+
}
1442+
if (cur->str) free(cur->str);
1443+
if (cur->name) free(cur->name);
1444+
free(cur);
1445+
cur = next;
1446+
}
1447+
*head = NULL;
1448+
}
1449+
1450+
static void
1451+
ff_cfg_free_port_one(struct ff_port_cfg *p)
1452+
{
1453+
if (p == NULL) return;
1454+
if (p->name) { free(p->name); p->name = NULL; }
1455+
if (p->ifname) { free(p->ifname); p->ifname = NULL; }
1456+
if (p->addr) { free(p->addr); p->addr = NULL; }
1457+
if (p->netmask) { free(p->netmask); p->netmask = NULL; }
1458+
if (p->broadcast) { free(p->broadcast); p->broadcast = NULL; }
1459+
if (p->gateway) { free(p->gateway); p->gateway = NULL; }
1460+
if (p->vip_ifname) { free(p->vip_ifname); p->vip_ifname = NULL; }
1461+
if (p->vip_addr_str) { free(p->vip_addr_str); p->vip_addr_str = NULL; }
1462+
if (p->vip_addr_array) {
1463+
/* IMPORTANT: vip_addr_array[i] are pointers INTO vip_addr_str
1464+
* (rte_strsplit splits in place; see vip_cfg_handler). We must
1465+
* only free the array container, NOT the element pointers. */
1466+
free(p->vip_addr_array);
1467+
p->vip_addr_array = NULL;
1468+
}
1469+
p->nb_vip = 0;
1470+
#ifdef FF_IPFW
1471+
if (p->pr_addr_str) { free(p->pr_addr_str); p->pr_addr_str = NULL; }
1472+
if (p->pr_cfg) { free(p->pr_cfg); p->pr_cfg = NULL; }
1473+
p->nb_pr = 0;
1474+
#endif
1475+
#ifdef INET6
1476+
if (p->addr6_str) { free(p->addr6_str); p->addr6_str = NULL; }
1477+
if (p->gateway6_str) { free(p->gateway6_str); p->gateway6_str = NULL; }
1478+
if (p->vip_addr6_str) { free(p->vip_addr6_str); p->vip_addr6_str = NULL; }
1479+
if (p->vip_addr6_array) {
1480+
/* Same alias-into-vip_addr6_str semantics as IPv4 above. */
1481+
free(p->vip_addr6_array);
1482+
p->vip_addr6_array = NULL;
1483+
}
1484+
p->nb_vip6 = 0;
1485+
#endif
1486+
if (p->slave_portid_list) {
1487+
free(p->slave_portid_list);
1488+
p->slave_portid_list = NULL;
1489+
}
1490+
/* p->vlan_cfgs[] entries point INTO ff_global_cfg.dpdk.vlan_cfgs[]
1491+
* (set by vlan_cfg_handler when binding a VLAN to its parent port).
1492+
* We must NOT free them here — they are freed once via the dpdk.vlan_cfgs
1493+
* array sweep below. */
1494+
}
1495+
1496+
static void
1497+
ff_cfg_free_vlan_one(struct ff_vlan_cfg *v)
1498+
{
1499+
if (v == NULL) return;
1500+
if (v->name) { free(v->name); v->name = NULL; }
1501+
if (v->ifname) { free(v->ifname); v->ifname = NULL; }
1502+
if (v->addr) { free(v->addr); v->addr = NULL; }
1503+
if (v->netmask) { free(v->netmask); v->netmask = NULL; }
1504+
if (v->broadcast) { free(v->broadcast); v->broadcast = NULL; }
1505+
if (v->gateway) { free(v->gateway); v->gateway = NULL; }
1506+
if (v->vip_ifname) { free(v->vip_ifname); v->vip_ifname = NULL; }
1507+
if (v->vip_addr_str) { free(v->vip_addr_str); v->vip_addr_str = NULL; }
1508+
#ifdef FF_IPFW
1509+
if (v->pr_addr_str) { free(v->pr_addr_str); v->pr_addr_str = NULL; }
1510+
#endif
1511+
#ifdef INET6
1512+
if (v->addr6_str) { free(v->addr6_str); v->addr6_str = NULL; }
1513+
if (v->gateway6_str) { free(v->gateway6_str); v->gateway6_str = NULL; }
1514+
if (v->vip_addr6_str) { free(v->vip_addr6_str); v->vip_addr6_str = NULL; }
1515+
#endif
1516+
}
1517+
1518+
static void
1519+
ff_cfg_free_vdev_one(struct ff_vdev_cfg *v)
1520+
{
1521+
if (v == NULL) return;
1522+
if (v->name) { free(v->name); v->name = NULL; }
1523+
if (v->iface) { free(v->iface); v->iface = NULL; }
1524+
if (v->path) { free(v->path); v->path = NULL; }
1525+
if (v->mac) { free(v->mac); v->mac = NULL; }
1526+
}
1527+
1528+
static void
1529+
ff_cfg_free_bond_one(struct ff_bond_cfg *b)
1530+
{
1531+
if (b == NULL) return;
1532+
if (b->name) { free(b->name); b->name = NULL; }
1533+
if (b->slave) { free(b->slave); b->slave = NULL; }
1534+
if (b->primary) { free(b->primary); b->primary = NULL; }
1535+
if (b->bond_mac) { free(b->bond_mac); b->bond_mac = NULL; }
1536+
if (b->xmit_policy) { free(b->xmit_policy); b->xmit_policy = NULL; }
1537+
}
1538+
1539+
void
1540+
ff_unload_config(void)
1541+
{
1542+
/* dpdk_argv[] entries (each strdup'd in dpdk_args_setup) */
1543+
for (int i = 0; i < dpdk_argc; i++) {
1544+
if (dpdk_argv[i]) {
1545+
free(dpdk_argv[i]);
1546+
dpdk_argv[i] = NULL;
1547+
}
1548+
}
1549+
dpdk_argc = 0;
1550+
1551+
/* Top-level filename + dpdk.* string fields */
1552+
if (ff_global_cfg.filename) {
1553+
free(ff_global_cfg.filename);
1554+
ff_global_cfg.filename = NULL;
1555+
}
1556+
if (ff_global_cfg.dpdk.proc_type) {
1557+
free(ff_global_cfg.dpdk.proc_type);
1558+
ff_global_cfg.dpdk.proc_type = NULL;
1559+
}
1560+
if (ff_global_cfg.dpdk.lcore_mask) {
1561+
free(ff_global_cfg.dpdk.lcore_mask);
1562+
ff_global_cfg.dpdk.lcore_mask = NULL;
1563+
}
1564+
if (ff_global_cfg.dpdk.proc_mask) {
1565+
free(ff_global_cfg.dpdk.proc_mask);
1566+
ff_global_cfg.dpdk.proc_mask = NULL;
1567+
}
1568+
if (ff_global_cfg.dpdk.base_virtaddr) {
1569+
free(ff_global_cfg.dpdk.base_virtaddr);
1570+
ff_global_cfg.dpdk.base_virtaddr = NULL;
1571+
}
1572+
if (ff_global_cfg.dpdk.file_prefix) {
1573+
free(ff_global_cfg.dpdk.file_prefix);
1574+
ff_global_cfg.dpdk.file_prefix = NULL;
1575+
}
1576+
if (ff_global_cfg.dpdk.allow) {
1577+
free(ff_global_cfg.dpdk.allow);
1578+
ff_global_cfg.dpdk.allow = NULL;
1579+
}
1580+
if (ff_global_cfg.dpdk.proc_lcore) {
1581+
free(ff_global_cfg.dpdk.proc_lcore);
1582+
ff_global_cfg.dpdk.proc_lcore = NULL;
1583+
}
1584+
if (ff_global_cfg.dpdk.portid_list) {
1585+
free(ff_global_cfg.dpdk.portid_list);
1586+
ff_global_cfg.dpdk.portid_list = NULL;
1587+
}
1588+
1589+
/* port_cfgs[] : array sized RTE_MAX_ETHPORTS */
1590+
if (ff_global_cfg.dpdk.port_cfgs) {
1591+
for (int i = 0; i < RTE_MAX_ETHPORTS; i++) {
1592+
ff_cfg_free_port_one(&ff_global_cfg.dpdk.port_cfgs[i]);
1593+
}
1594+
free(ff_global_cfg.dpdk.port_cfgs);
1595+
ff_global_cfg.dpdk.port_cfgs = NULL;
1596+
}
1597+
1598+
/* vlan_cfgs[] : array sized DPDK_MAX_VLAN_FILTER */
1599+
if (ff_global_cfg.dpdk.vlan_cfgs) {
1600+
for (int i = 0; i < DPDK_MAX_VLAN_FILTER; i++) {
1601+
ff_cfg_free_vlan_one(&ff_global_cfg.dpdk.vlan_cfgs[i]);
1602+
}
1603+
free(ff_global_cfg.dpdk.vlan_cfgs);
1604+
ff_global_cfg.dpdk.vlan_cfgs = NULL;
1605+
}
1606+
1607+
/* vdev_cfgs[] : array sized RTE_MAX_ETHPORTS */
1608+
if (ff_global_cfg.dpdk.vdev_cfgs) {
1609+
for (int i = 0; i < RTE_MAX_ETHPORTS; i++) {
1610+
ff_cfg_free_vdev_one(&ff_global_cfg.dpdk.vdev_cfgs[i]);
1611+
}
1612+
free(ff_global_cfg.dpdk.vdev_cfgs);
1613+
ff_global_cfg.dpdk.vdev_cfgs = NULL;
1614+
}
1615+
1616+
/* bond_cfgs[] : array sized RTE_MAX_ETHPORTS */
1617+
if (ff_global_cfg.dpdk.bond_cfgs) {
1618+
for (int i = 0; i < RTE_MAX_ETHPORTS; i++) {
1619+
ff_cfg_free_bond_one(&ff_global_cfg.dpdk.bond_cfgs[i]);
1620+
}
1621+
free(ff_global_cfg.dpdk.bond_cfgs);
1622+
ff_global_cfg.dpdk.bond_cfgs = NULL;
1623+
}
1624+
1625+
/* rss_check_cfgs : single struct */
1626+
if (ff_global_cfg.dpdk.rss_check_cfgs) {
1627+
if (ff_global_cfg.dpdk.rss_check_cfgs->rss_tbl_str) {
1628+
free((void *)ff_global_cfg.dpdk.rss_check_cfgs->rss_tbl_str);
1629+
ff_global_cfg.dpdk.rss_check_cfgs->rss_tbl_str = NULL;
1630+
}
1631+
free(ff_global_cfg.dpdk.rss_check_cfgs);
1632+
ff_global_cfg.dpdk.rss_check_cfgs = NULL;
1633+
}
1634+
1635+
/* kni.* string fields */
1636+
if (ff_global_cfg.kni.kni_action) {
1637+
free(ff_global_cfg.kni.kni_action);
1638+
ff_global_cfg.kni.kni_action = NULL;
1639+
}
1640+
if (ff_global_cfg.kni.method) {
1641+
free(ff_global_cfg.kni.method);
1642+
ff_global_cfg.kni.method = NULL;
1643+
}
1644+
if (ff_global_cfg.kni.tcp_port) {
1645+
free(ff_global_cfg.kni.tcp_port);
1646+
ff_global_cfg.kni.tcp_port = NULL;
1647+
}
1648+
if (ff_global_cfg.kni.udp_port) {
1649+
free(ff_global_cfg.kni.udp_port);
1650+
ff_global_cfg.kni.udp_port = NULL;
1651+
}
1652+
1653+
/* pcap.save_path */
1654+
if (ff_global_cfg.pcap.save_path) {
1655+
free(ff_global_cfg.pcap.save_path);
1656+
ff_global_cfg.pcap.save_path = NULL;
1657+
}
1658+
1659+
/* log.dir : ff_default_config strdup's the literal default; the INI
1660+
* parser may overwrite with another strdup (which leaks the default).
1661+
* That overwrite leak is fixed by the strdup_replace pattern in
1662+
* ini_parse_handler -- here we just free whatever heap pointer survives. */
1663+
if (ff_global_cfg.log.dir) {
1664+
free(ff_global_cfg.log.dir);
1665+
ff_global_cfg.log.dir = NULL;
1666+
}
1667+
1668+
/* freebsd.boot / freebsd.sysctl : linked-list chains */
1669+
ff_cfg_free_freebsd_chain(&ff_global_cfg.freebsd.boot);
1670+
ff_cfg_free_freebsd_chain(&ff_global_cfg.freebsd.sysctl);
1671+
}

‎lib/ff_config.h‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,20 @@ extern struct ff_config ff_global_cfg;
346346

347347
int ff_load_config(int argc, char * const argv[]);
348348

349+
/*
350+
* Free every heap-allocated field referenced by ff_global_cfg (port_cfgs,
351+
* vlan_cfgs, vdev_cfgs, bond_cfgs, rss_check_cfgs, freebsd.boot/sysctl
352+
* linked lists, dpdk_argv[], filename, etc.) and zero the structure so a
353+
* subsequent ff_load_config() call starts from a clean slate.
354+
*
355+
* Registered via atexit() at the end of the first successful
356+
* ff_load_config(), and also invoked at the start of any later
357+
* ff_load_config() to make the loader idempotent (FU-S2-2-CFG-UNLOAD).
358+
*
359+
* Safe to call multiple times; calling on an already-zeroed cfg is a no-op.
360+
*/
361+
void ff_unload_config(void);
362+
349363
#ifdef __cplusplus
350364
}
351365
#endif

‎lib/ff_dpdk_if.c‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2508,7 +2508,11 @@ ff_dpdk_run(loop_func_t loop, void *arg) {
25082508
stop_clock();
25092509
rte_free(lr);
25102510

2511-
/* FIXME: Cleanup ff_config, freebsd etc. */
2511+
/* FU-S2-2-CFG-UNLOAD: free ff_global_cfg before EAL cleanup so any
2512+
* cfg.* allocations made via DPDK helpers (none today, but defensive)
2513+
* are released while the EAL is still up. Mirrors the comment block
2514+
* that previously read "FIXME: Cleanup ff_config, freebsd etc." */
2515+
ff_unload_config();
25122516
rte_eal_cleanup();
25132517
ff_log_close();
25142518
}

0 commit comments

Comments
 (0)