Skip to content

Commit

Permalink
fsmonitor-settings: use xstrdup(), don't strdup() twice
Browse files Browse the repository at this point in the history
Fix a memory leak and lack of error checking in
1e0ea5c (fsmonitor: config settings are repository-specific,
2022-03-25). Anything calling repo_config_get_pathname() will get a
strbuf_detach()'d string, so by using strdup() unconditionally we'd
needlessly duplicate a string that was already duplicated for us.

This was seemingly being done because fsm_settings__set_hook() wanted
to be a generic function that could take a path from anywhere, and
even if it was only used within fsmonitor-settings.c it would also
need to duplicate strings in the getenv() case.

Then we didn't check the return value of strdup(), let's use xstrdup()
instead.

To fix this we'd need to pass information down to
fsm_settings__set_hook() about whether to duplicate the string, but
since nothing except this file uses it we can at least make it static.

But if we were going to do that it wouldn't make any sense to keep the
"!r" fallback case, we know we have "r" in
lookup_fsmonitor_settings(). Nor would it make sense to have these
"set" functions call lookup_fsmonitor_settings(), since their only
caller is lookup_fsmonitor_settings() itself.

And as we know that that's all true the FREE_AND_NULL() don't make any
sense either, we'd only want that if we were clearing a previously set
hook, but due to the same guard clause in lookup_fsmonitor_settings()
that's keeping us from recusing infinitely we know that we won't ever
re-set these values once they're set.

So all of this was quite a a lot of boilerplate just to set a couple
of variables, let's just do that in lookup_fsmonitor_settings()
instead.

While we're at it let's remove the fsm_settings__set_disabled()
sibling method, it wasn't used by anything.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
  • Loading branch information
avar committed Apr 22, 2022
1 parent 6ac7a97 commit 4efdd4f
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 40 deletions.
41 changes: 5 additions & 36 deletions fsmonitor-settings.c
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ static void lookup_fsmonitor_settings(struct repository *r)
{
struct fsmonitor_settings *s;
const char *const_str;
int dup_str = 0;
int bool_value;

if (r->settings.fsmonitor)
Expand All @@ -37,11 +38,12 @@ static void lookup_fsmonitor_settings(struct repository *r)

case 0: /* config value was set to <bool> */
if (bool_value)
fsm_settings__set_ipc(r);
s->mode = FSMONITOR_MODE_IPC;
return;

case 1: /* config value was unset */
const_str = getenv("GIT_TEST_FSMONITOR");
dup_str = 1;
break;

case -1: /* config value set to an arbitrary string */
Expand All @@ -56,7 +58,8 @@ static void lookup_fsmonitor_settings(struct repository *r)
if (!const_str || !*const_str)
return;

fsm_settings__set_hook(r, const_str);
s->mode = FSMONITOR_MODE_HOOK;
s->hook_path = dup_str ? xstrdup(const_str) : (char *)const_str;;
}

enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
Expand All @@ -78,37 +81,3 @@ const char *fsm_settings__get_hook_path(struct repository *r)

return r->settings.fsmonitor->hook_path;
}

void fsm_settings__set_ipc(struct repository *r)
{
if (!r)
r = the_repository;

lookup_fsmonitor_settings(r);

r->settings.fsmonitor->mode = FSMONITOR_MODE_IPC;
FREE_AND_NULL(r->settings.fsmonitor->hook_path);
}

void fsm_settings__set_hook(struct repository *r, const char *path)
{
if (!r)
r = the_repository;

lookup_fsmonitor_settings(r);

r->settings.fsmonitor->mode = FSMONITOR_MODE_HOOK;
FREE_AND_NULL(r->settings.fsmonitor->hook_path);
r->settings.fsmonitor->hook_path = strdup(path);
}

void fsm_settings__set_disabled(struct repository *r)
{
if (!r)
r = the_repository;

lookup_fsmonitor_settings(r);

r->settings.fsmonitor->mode = FSMONITOR_MODE_DISABLED;
FREE_AND_NULL(r->settings.fsmonitor->hook_path);
}
4 changes: 0 additions & 4 deletions fsmonitor-settings.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,6 @@ enum fsmonitor_mode {
FSMONITOR_MODE_IPC = 2, /* core.fsmonitor=<true> */
};

void fsm_settings__set_ipc(struct repository *r);
void fsm_settings__set_hook(struct repository *r, const char *path);
void fsm_settings__set_disabled(struct repository *r);

enum fsmonitor_mode fsm_settings__get_mode(struct repository *r);
const char *fsm_settings__get_hook_path(struct repository *r);

Expand Down

0 comments on commit 4efdd4f

Please sign in to comment.