Fix global repository field not being cleared #4083
Conversation
|
Hi @zwerdna, thank you for contributing to Git for Windows.
I am really curious about this. Could you provide details, please? Also, please sign-off on your patch, it will need to go to upstream Git and they require this. |
sceptical-coder
force-pushed
the
fix-uncleared-commondir
branch
from
5ebfe96
to
3e6bef6
Compare
The mentioned check is here: Line 784 in 207275f The call stack to it is as follows: |
|
BTW, are there any unstable CI pipelines here or the issue in the above ones should be properly investigated in an obligatory order? |
sceptical-coder
force-pushed
the
fix-uncleared-commondir
branch
2 times, most recently
from
1760bca
to
b234671
Compare
Answering myself: found PR with pipeline fix. |
Could you please include this information in the commit message? For inspiration how to write excellent Git commit messages, please follow these guidelines: https://github.blog/2022-06-30-write-better-commits-build-better-projects/ |
sceptical-coder
force-pushed
the
fix-uncleared-commondir
branch
from
b234671
to
aff2991
Compare
|
Please, have a look again. I tried hard to clarify the commit's message now. |
sceptical-coder
force-pushed
the
fix-uncleared-commondir
branch
2 times, most recently
from
55563c7
to
d7161b7
Compare
Sorry, forgotten about Git interpreting hash signs in commit message as comments, and so got my commit message completely ruined w.r.t. backtraces. Now that is fixed. |
sceptical-coder
force-pushed
the
fix-uncleared-commondir
branch
2 times, most recently
from
675e455
to
6068b26
Compare
Regarding this: haven't found the commit 61cbb768c1f with aforementioned check in gitgitgadget/git repo, do you still think it's relevant to upstream Git? |
sceptical-coder
force-pushed
the
fix-uncleared-commondir
branch
from
6068b26
to
4ae630e
Compare
|
Good point! So we need to squash it before upstreaming that commit. @zwerdna would you mind annotating the commit message accordingly (i.e. using |
sceptical-coder
force-pushed
the
fix-uncleared-commondir
branch
from
4ae630e
to
3448194
Compare
Done, via |
sceptical-coder
force-pushed
the
fix-uncleared-commondir
branch
3 times, most recently
from
df38aa7
to
cb51ac9
Compare
| @@ -46,7 +46,7 @@ static void repo_set_commondir(struct repository *repo, | |||
| { | |||
| struct strbuf sb = STRBUF_INIT; | |||
|
|
|||
| free(repo->commondir); | |||
| FREE_AND_NULL(repo->commondir); | |||
There was a problem hiding this comment.
Wait, didn't you say that the commit that introduced the problem was not yet upstreamed? But I see gitgitgadget@b0f1a41 being the identical patch, but targeting upstream's default branch...
There was a problem hiding this comment.
Yes, actually I first thought that it will be applicable there, but then started to investigate further and came to conclusion that it quite probably would not be relevant there and closed my PR.
Anyway, I've seen that you hinted in that PR something like that it still can be important in upstream, so I'll read your comments more thoroughly and then decide whether to continue working with upstream instead.
There was a problem hiding this comment.
The bug described is not present in upstream (at least in the exact same scenario).
There was a problem hiding this comment.
I am actually rather convinced that the upstream contribution is the way we should go forward with: the patch is clearly fixing a bug that is caused by the combination of two upstream patches, and even if the current symptom cannot be seen with upstream's version, it is still a bug in upstream.
There was a problem hiding this comment.
Could you elaborate, please? I'm not aware of implicit assumptions taken in the codebase, and for me it's quite likely that they are implying that the code path in question should not request for config values of the global repository object... If that's not the case, I would be happy to make an upstream contribution with corresponding reasoning.
There was a problem hiding this comment.
Could you elaborate, please?
In gitgitgadget@f9b7573#diff-8107da631aae41d1abfe44a6e2ee403d79b2bdf07398eb1f7737fefeb073c0acR43, code was added that releases the memory that repo->commondir points to, but it does not set that pointer to NULL because the very next line sets it to something different.
However, in gitgitgadget@357a03e#diff-8107da631aae41d1abfe44a6e2ee403d79b2bdf07398eb1f7737fefeb073c0acR59-R66, quite a bit of code (and a call to another function) was introduced between releasing that memory and setting it again.
This introduced the fragility in the code that eventually lead to the code path (currently only a problem in Git for Windows' fork).
One might argue that the bug is in the windows.appendAtomically code, but this would be an incorrect assessment: the fragile code was introduced by 357a03e and it was only a matter of time until a patch would trigger this fragility, in this instance 61cbb76.
What 61cbb76 does is totally legitimate: it asks whether there is a repo->commondir to work with, and if there is, uses it. It's the fault of 357a03e that there is an unusable, non-NULL repo->commondir.
There was a problem hiding this comment.
Thanks, exactly what I thought (actually). I'll prepare the PR to upstream and get this discussion linked in it.
There was a problem hiding this comment.
I'll prepare the PR to upstream
@zwerdna you could simply reopen gitgitgadget#1402 :-)
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes git-for-windows#4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
fix-uncleared-commondir
branch
from
cb51ac9
to
5081e28
Compare
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes #4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that `free()`'d it.
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes #4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that `free()`'d it.
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes #4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that `free()`'d it.
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes #4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that `free()`'d it.
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes #4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that `free()`'d it.
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes #4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that `free()`'d it.
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes #4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that `free()`'d it.
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes #4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that `free()`'d it.
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes #4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that `free()`'d it.
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes #4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that `free()`'d it.
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes #4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes #4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that `free()`'d it.
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that `free()`'d it.
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes #4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that `free()`'d it.
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes #4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that `free()`'d it.
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes git-for-windows#4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that `free()`'d it.
In f9b7573 (repository: free fields before overwriting them, 2017-09-05), Git was taught to release memory before overwriting it, but 357a03e (repository.c: move env-related setup code back to environment.c, 2018-03-03) changed the code so that it would not _always_ be overwritten. As a consequence, the `commondir` attribute would point to already-free()d memory. This seems not to cause problems in core Git, but there are add-on patches in Git for Windows where the `commondir` attribute is subsequently used and causing invalid memory accesses e.g. in setups containing old-style submodules (i.e. the ones with a `.git` directory within theirs worktrees) that have `commondir` configured. This fixes #4083. Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that `free()`'d it.
It is checked for w.r.t. global repository struct down in the callstack in compatibility layer for MinGW before being assigned in the function that
free()'d it.