Skip to content
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

Make sure tools never create config.ldb #7551

Closed
wants to merge 4 commits into from
Closed

Make sure tools never create config.ldb #7551

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ce0b070
TOOLS: skip confdb_init if no context ptr provided
alexey-tikhonov Aug 30, 2024
c78f328
TOOLS: get rid of code duplication
alexey-tikhonov Aug 30, 2024
12d38cc
TOOLS: use `sss_tool_confdb_init()` everywhere
alexey-tikhonov Aug 30, 2024
0c3d9a0
CONFDB: move sanity check
alexey-tikhonov Sep 4, 2024
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
12 changes: 6 additions & 6 deletions src/tools/common/sss_tools.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,15 @@ static void sss_tool_common_opts(struct sss_tool_ctx *tool_ctx,
static errno_t sss_tool_confdb_init(TALLOC_CTX *mem_ctx,
struct confdb_ctx **_confdb)
{
struct confdb_ctx *confdb;
char *path;
errno_t ret;
struct stat statbuf;

if (_confdb == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE, "Bad argument\n");
return EFAULT;
}

path = talloc_asprintf(mem_ctx, "%s/%s", DB_PATH, CONFDB_FILE);
if (path == NULL) {
return ENOMEM;
Expand All @@ -108,18 +112,14 @@ static errno_t sss_tool_confdb_init(TALLOC_CTX *mem_ctx,
return ret;
}

ret = confdb_init(mem_ctx, &confdb, path);
ret = confdb_init(mem_ctx, _confdb, path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not directly related to you commit.

confdb_init() is assuming its cdb_ctx parameter is not NULL. It clearly is not here, but could be in other calls. Shouldn't we also check inside this function that it is not NULL? Or maybe just move the check to that function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't we also check inside this function that it is not NULL?

this == sss_tool_confdb_init()?

But here is the check in place at the beginning (line 97)
Or do I misunderstand?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, sorry. this == confdb_init().

In other words, sss_tool_confdb_init() is checking _confdb != NULL, but other functions are also calling confdb_init(), and those functions may or may not check that condition. It might be better to move the check inside confdb_init().

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, added one more patch.

talloc_zfree(path);
if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE, "Unable to connect to config DB [%d]: %s\n",
ret, sss_strerror(ret));
return ret;
}

if (_confdb != NULL) {
*_confdb = confdb;
}

return EOK;
}

Expand Down