* Re: Bug - remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL
2022-05-19 6:58 ` Jeff King
@ 2022-05-28 0:12 ` Glen Choo
2022-05-28 0:13 ` Glen Choo
1 sibling, 0 replies; 4+ messages in thread
From: Glen Choo @ 2022-05-28 0:12 UTC (permalink / raw)
To: Jeff King, Ing. Martin Prantl Ph.D.; +Cc: git
Thanks for taking a look! (and welcome back :))
Jeff King <peff@peff.net> writes:
> On Thu, May 19, 2022 at 08:23:25AM +0200, Ing. Martin Prantl Ph.D. wrote:
>
>> file:.git/config branch..remote=origin
>> file:.git/config branch..merge=refs/heads/
>> [...]
>>
>> git ls-remote
>> BUG: remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL
>
> Those branch entries with an empty subsection are the culprit. I'm not
> sure how they got there, but they should be safe to remove, which will
> make your immediate problem go away.
>
> It looks like handling of such bogus keys regressed in 4a2dcb1a08
> (remote: die if branch is not found in repository, 2021-11-17). In
> make_branch(), the call to find_branch() gets confused by the 0-length
> "len" parameter, and instead uses strlen() on the partial string
> containing the rest of the config key. So it tries to look up branch
> ".remote" for the first key, and ".merge" for the second. Since neither
> exist, in both cases it then tries to add a new entry, but this time
> correctly using the 0-length string. Which will confusingly already be
> present when handling the second key.
It wasn't obvious to me before what the regression was (since a 0-length
branch name is nonsense, right?). Turns out that we used to just ignore
the 0-length branch name, but now we BUG(), so yeah this needs fixing.
Interestingly, this 'name=".remote" and len=0 confusion' pre-dates that
commit, but it got exposed when that commit introduced the confused hash
map.
I can get the old behavior by getting rid of the strlen() fallback (I
think I will, it doesn't provide any benefit AFAICT), but...
> Either find_branch() needs to become more careful about distinguishing
> the two cases, or perhaps 0-length names should be rejected earlier (I
> don't think they could ever be useful).
I think this is even better. Warning the user about their bad config
sounds like a good thing.
We would have to be careful not to reject an empty 'name', because this
might be a non-subsection branch config, e.g. branch.autoSetupRebase.
Something like..
diff --git a/remote.c b/remote.c
index a1463aefb7..d3ae1445a4 100644
--- a/remote.c
+++ b/remote.c
@@ -351,8 +351,12 @@ static int handle_config(const char *key, const char *value, void *cb)
struct remote_state *remote_state = cb;
if (parse_config_key(key, "branch", &name, &namelen, &subkey) >= 0) {
+ /* There is no subsection. */
if (!name)
return 0;
+ /* There is a subsection, but it is empty. */
+ if (!namelen)
+ return -1;
branch = make_branch(remote_state, name, namelen);
if (!strcmp(subkey, "remote")) {
return git_config_string(&branch->remote_name, key, value);
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: Bug - remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL
2022-05-19 6:58 ` Jeff King
2022-05-28 0:12 ` Glen Choo
@ 2022-05-28 0:13 ` Glen Choo
1 sibling, 0 replies; 4+ messages in thread
From: Glen Choo @ 2022-05-28 0:13 UTC (permalink / raw)
To: Jeff King, Ing. Martin Prantl Ph.D.; +Cc: git
Thanks for taking a look! (and welcome back :))
Jeff King <peff@peff.net> writes:
> On Thu, May 19, 2022 at 08:23:25AM +0200, Ing. Martin Prantl Ph.D. wrote:
>
>> file:.git/config branch..remote=origin
>> file:.git/config branch..merge=refs/heads/
>> [...]
>>
>> git ls-remote
>> BUG: remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL
>
> Those branch entries with an empty subsection are the culprit. I'm not
> sure how they got there, but they should be safe to remove, which will
> make your immediate problem go away.
>
> It looks like handling of such bogus keys regressed in 4a2dcb1a08
> (remote: die if branch is not found in repository, 2021-11-17). In
> make_branch(), the call to find_branch() gets confused by the 0-length
> "len" parameter, and instead uses strlen() on the partial string
> containing the rest of the config key. So it tries to look up branch
> ".remote" for the first key, and ".merge" for the second. Since neither
> exist, in both cases it then tries to add a new entry, but this time
> correctly using the 0-length string. Which will confusingly already be
> present when handling the second key.
It wasn't obvious to me before what the regression was (since a 0-length
branch name is nonsense, right?). Turns out that we used to just ignore
the 0-length branch name, but now we BUG(), so yeah this needs fixing.
Interestingly, this 'name=".remote" and len=0 confusion' pre-dates that
commit, but it got exposed when that commit introduced the confused hash
map.
I can get the old behavior by getting rid of the strlen() fallback (I
think I will, it doesn't provide any benefit AFAICT), but...
> Either find_branch() needs to become more careful about distinguishing
> the two cases, or perhaps 0-length names should be rejected earlier (I
> don't think they could ever be useful).
I think this is even better. Warning the user about their bad config
sounds like a good thing.
We would have to be careful not to reject an empty 'name', because this
might be a non-subsection config that starts with "branch.", e.g.
branch.autoSetupRebase. Something like..
diff --git a/remote.c b/remote.c
index a1463aefb7..d3ae1445a4 100644
--- a/remote.c
+++ b/remote.c
@@ -351,8 +351,12 @@ static int handle_config(const char *key, const char *value, void *cb)
struct remote_state *remote_state = cb;
if (parse_config_key(key, "branch", &name, &namelen, &subkey) >= 0) {
+ /* There is no subsection. */
if (!name)
return 0;
+ /* There is a subsection, but it is empty. */
+ if (!namelen)
+ return -1;
branch = make_branch(remote_state, name, namelen);
if (!strcmp(subkey, "remote")) {
return git_config_string(&branch->remote_name, key, value);
^ permalink raw reply related [flat|nested] 4+ messages in thread