Skip to content

tree: fix mem leaks in multiple nvme ctrl routines #1028

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
martin-gpy wants to merge 1 commit into from
Closed

tree: fix mem leaks in multiple nvme ctrl routines #1028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 133 additions & 15 deletions src/nvme/tree.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1220,6 +1220,126 @@ void nvme_ctrl_set_tls_key(nvme_ctrl_t c, const char *key)
c->tls_key = strdup(key);
}

void nvme_ctrl_set_addr(nvme_ctrl_t c, const char *addr)
{
if (c->address) {
free(c->address);
Copy link
Copy Markdown
Contributor

@martin-belanger martin-belanger Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

free() is NULL-safe. So you could just call it w/o first cheking that the pointer in non-NULL.

https://man7.org/linux/man-pages/man3/free.3p.html

Copy link
Copy Markdown
Contributor Author

@martin-gpy martin-gpy Jun 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Guess this should be cleaned up in other parts of the code too.

c->address = NULL;
}
if (addr)
c->address = strdup(addr);
}

void nvme_ctrl_set_name(nvme_ctrl_t c, const char *name)
{
if (c->name) {
free(c->name);
c->name = NULL;
}
if (name)
c->name = strdup(name);
}

void nvme_ctrl_set_sysfs_dir(nvme_ctrl_t c, const char *sysfs_dir)
{
if (c->sysfs_dir) {
free(c->sysfs_dir);
c->sysfs_dir = NULL;
}
if (sysfs_dir)
c->sysfs_dir = strdup(sysfs_dir);
}

void nvme_ctrl_set_firmware(nvme_ctrl_t c)
{
if (c->firmware) {
free(c->firmware);
c->firmware = NULL;
}
c->firmware = nvme_get_ctrl_attr(c, "firmware_rev");
}

void nvme_ctrl_set_model(nvme_ctrl_t c)
{
if (c->model) {
free(c->model);
c->model = NULL;
}
c->model = nvme_get_ctrl_attr(c, "model");
}

void nvme_ctrl_set_state(nvme_ctrl_t c)
{
if (c->state) {
free(c->state);
c->state = NULL;
}
c->state = nvme_get_ctrl_attr(c, "state");
}

void nvme_ctrl_set_numa_node(nvme_ctrl_t c)
{
if (c->numa_node) {
free(c->numa_node);
c->numa_node = NULL;
}
c->numa_node = nvme_get_ctrl_attr(c, "numa_node");
}

void nvme_ctrl_set_queue_count(nvme_ctrl_t c)
{
if (c->queue_count) {
free(c->queue_count);
c->queue_count = NULL;
}
c->queue_count = nvme_get_ctrl_attr(c, "queue_count");
}

void nvme_ctrl_set_serial(nvme_ctrl_t c)
{
if (c->serial) {
free(c->serial);
c->serial = NULL;
}
c->serial = nvme_get_ctrl_attr(c, "serial");
}

void nvme_ctrl_set_sqsize(nvme_ctrl_t c)
{
if (c->sqsize) {
free(c->sqsize);
c->sqsize = NULL;
}
c->sqsize = nvme_get_ctrl_attr(c, "sqsize");
}

void nvme_ctrl_set_cntrltype(nvme_ctrl_t c)
{
if (c->cntrltype) {
free(c->cntrltype);
c->cntrltype = NULL;
}
c->cntrltype = nvme_get_ctrl_attr(c, "cntrltype");
}

void nvme_ctrl_set_cntlid(nvme_ctrl_t c)
{
if (c->cntlid) {
free(c->cntlid);
c->cntlid = NULL;
}
c->cntlid = nvme_get_ctrl_attr(c, "cntlid");
}

void nvme_ctrl_set_dctype(nvme_ctrl_t c)
{
if (c->dctype) {
free(c->dctype);
c->dctype = NULL;
}
c->dctype = nvme_get_ctrl_attr(c, "dctype");
}

void nvme_ctrl_set_discovered(nvme_ctrl_t c, bool discovered)
{
c->discovered = discovered;
Expand Down Expand Up @@ -2033,18 +2153,18 @@ static int nvme_configure_ctrl(nvme_root_t r, nvme_ctrl_t c, const char *path,
closedir(d);

c->fd = -1;
c->name = strdup(name);
c->sysfs_dir = (char *)path;
c->firmware = nvme_get_ctrl_attr(c, "firmware_rev");
c->model = nvme_get_ctrl_attr(c, "model");
c->state = nvme_get_ctrl_attr(c, "state");
c->numa_node = nvme_get_ctrl_attr(c, "numa_node");
c->queue_count = nvme_get_ctrl_attr(c, "queue_count");
c->serial = nvme_get_ctrl_attr(c, "serial");
c->sqsize = nvme_get_ctrl_attr(c, "sqsize");
c->cntrltype = nvme_get_ctrl_attr(c, "cntrltype");
c->cntlid = nvme_get_ctrl_attr(c, "cntlid");
c->dctype = nvme_get_ctrl_attr(c, "dctype");
nvme_ctrl_set_name(c, name);
nvme_ctrl_set_sysfs_dir(c, path);
nvme_ctrl_set_firmware(c);
nvme_ctrl_set_model(c);
nvme_ctrl_set_state(c);
nvme_ctrl_set_numa_node(c);
nvme_ctrl_set_queue_count(c);
nvme_ctrl_set_serial(c);
nvme_ctrl_set_sqsize(c);
nvme_ctrl_set_cntrltype(c);
nvme_ctrl_set_cntlid(c);
nvme_ctrl_set_dctype(c);
c->phy_slot = nvme_ctrl_lookup_phy_slot(r, c->address);
nvme_read_sysfs_dhchap(r, c);
nvme_read_sysfs_tls(r, c);
Expand Down Expand Up @@ -2194,8 +2314,7 @@ static nvme_ctrl_t nvme_ctrl_alloc(nvme_root_t r, nvme_subsystem_t s,
errno = ENODEV;
return NULL;
}
c->address = addr;
addr = NULL;
nvme_ctrl_set_addr(c, addr);
if (s->subsystype && !strcmp(s->subsystype, "discovery"))
c->discovery_ctrl = true;
ret = nvme_configure_ctrl(r, c, path, name);
Expand Down Expand Up @@ -2263,7 +2382,6 @@ nvme_ctrl_t nvme_scan_ctrl(nvme_root_t r, const char *name)
if (!c)
return NULL;

path = NULL;
Copy link
Copy Markdown
Collaborator

@igaw igaw Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated.

Copy link
Copy Markdown
Contributor Author

@martin-gpy martin-gpy Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why unrelated? path already gets freed and set to NULL in the helper function in my patch. So there was no need to separately set it to NULL here to avoid getting it freed through the cleanup macro. Otherwise this would have resulted in a double free.

Copy link
Copy Markdown
Collaborator

@igaw igaw Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah didn't realize there is the cleanup hook. Hmm I think we should avoid passing owner ship like this around, it leads to more problems in the long run. If nvme_ctrl_alloc needs the path variable it should duplicate it. Let me look into it.

nvme_ctrl_scan_paths(r, c);
nvme_ctrl_scan_namespaces(r, c);
return c;
Expand Down
81 changes: 81 additions & 0 deletions src/nvme/tree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1168,6 +1168,87 @@ const char *nvme_ctrl_get_tls_key(nvme_ctrl_t c);
*/
void nvme_ctrl_set_tls_key(nvme_ctrl_t c, const char *key);

/**
* nvme_ctrl_set_addr() - Set controller address
* @c: Controller whose address to set
* @addr: Address
*/
void nvme_ctrl_set_addr(nvme_ctrl_t c, const char *addr);
Copy link
Copy Markdown
Collaborator

@igaw igaw Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these function should be exposed at all.

Copy link
Copy Markdown
Contributor Author

@martin-gpy martin-gpy Jun 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying these pointers should be freed within nvme_configure_ctrl() & nvme_ctrl_alloc() itself? That would make the code ugly to read though...

Copy link
Copy Markdown
Collaborator

@igaw igaw Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still reading through the code. Not sure what is best, but I really don't want these setters in the public API.


/**
* nvme_ctrl_set_name() - Set controller name
* @c: Controller whose name to set
* @name: Name
*/
void nvme_ctrl_set_name(nvme_ctrl_t c, const char *name);

/**
* nvme_ctrl_set_sysfs_dir() - Set controller sysfs_dir
* @c: Controller whose sysfs_dir to set
* @sysfs_dir: Sysfs_dir
*/
void nvme_ctrl_set_sysfs_dir(nvme_ctrl_t c, const char *sysfs_dir);

/**
* nvme_ctrl_set_firmware() - Set controller firmware_rev
* @c: Controller whose firmware revision to set
*/
void nvme_ctrl_set_firmware(nvme_ctrl_t c);

/**
* nvme_ctrl_set_model() - Set controller model
* @c: Controller whose model to set
*/
void nvme_ctrl_set_model(nvme_ctrl_t c);

/**
* nvme_ctrl_set_state() - Set controller state
* @c: Controller whose state to set
*/
void nvme_ctrl_set_state(nvme_ctrl_t c);

/**
* nvme_ctrl_set_numa_node() - Set controller numa_node
* @c: Controller whose numa_node to set
*/
void nvme_ctrl_set_numa_node(nvme_ctrl_t c);

/**
* nvme_ctrl_set_queue_count() - Set controller queue_count
* @c: Controller whose queue_count to set
*/
void nvme_ctrl_set_queue_count(nvme_ctrl_t c);

/**
* nvme_ctrl_set_serial() - Set controller serial
* @c: Controller whose serial to set
*/
void nvme_ctrl_set_serial(nvme_ctrl_t c);

/**
* nvme_ctrl_set_sqsize() - Set controller sqsize
* @c: Controller whose sqsize to set
*/
void nvme_ctrl_set_sqsize(nvme_ctrl_t c);

/**
* nvme_ctrl_set_cntrltype() - Set controller cntrltype
* @c: Controller whose cntrltype to set
*/
void nvme_ctrl_set_cntrltype(nvme_ctrl_t c);

/**
* nvme_ctrl_set_cntlid() - Set controller cntlid
* @c: Controller whose cntlid to set
*/
void nvme_ctrl_set_cntlid(nvme_ctrl_t c);

/**
* nvme_ctrl_set_dctype() - Set controller dctype
* @c: Controller whose dctype to set
*/
void nvme_ctrl_set_dctype(nvme_ctrl_t c);

/**
* nvme_ctrl_get_config() - Fabrics configuration of a controller
* @c: Controller instance
Expand Down