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

Jump to bottom

Fix bugs related to real_path() #575

Closed
wants to merge 4 commits into from
Closed

Fix bugs related to real_path() #575

wants to merge 4 commits into from

Conversation

SyntevoAlex
Copy link

@SyntevoAlex SyntevoAlex commented Mar 6, 2020 •
edited 

Changes since V1
-------------------
1) Removed `strbuf_realpath()` that weren't needed
2) Code style in declaration of `get_superproject_working_tree()`

Original description
-------------------
The issue with `real_path()` seems to be long-standing, where multiple
people solved parts of it over time. I'm adding another part here
after I have discovered a crash related to it.

Even with this step, there are still problems remaining:
* `read_gitfile_gently()` still uses shared buffer.
* `absolute_path()` was not removed.

These issues remain because there're too many code references and I'd like
to avoid submitting a single topic of a scary size.

Cc: Junio C Hamano <gitster@pobox.com>

@SyntevoAlex
set_git_dir: fix crash when used with real_path()
f7afcb4 
`real_path()` returns result from a shared buffer, inviting subtle
reentrance bugs. One of these bugs occur when invoked this way:
    set_git_dir(real_path(git_dir))

In this case, `real_path()` has reentrance:
    real_path
    read_gitfile_gently
    repo_set_gitdir
    setup_git_env
    set_git_dir_1
    set_git_dir

Later, `set_git_dir()` uses its now-dead parameter:
    !is_absolute_path(path)

Fix this by using a dedicated `strbuf` to hold `strbuf_realpath()`.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
@SyntevoAlex SyntevoAlex changed the title #0205(git) crash real path Fix bugs related to real_path() Mar 6, 2020
@SyntevoAlex SyntevoAlex force-pushed the #0205(git)_crash_real_path branch 3 times, most recently from 3a6a0d0 to 2eeefda Compare March 6, 2020 18:33
@SyntevoAlex
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 6, 2020

Submitted as pull.575.git.1583521396.gitgitgadget@gmail.com

gitgitgadget[bot]
gitgitgadget bot reviewed Mar 6, 2020
View reviewed changes
abspath.c Outdated Show resolved Hide resolved
gitgitgadget[bot]
gitgitgadget bot reviewed Mar 6, 2020
View reviewed changes
abspath.c Show resolved Hide resolved
@dscho
Copy link
Member

dscho commented Mar 6, 2020

Judging from the log, I think that CirrusCI might stumble over your naming convention using hash symbols and parentheses:

Using built-in Git...

Limiting clone depth to 1!
Cloning refs/tags/pr-575/SyntevoAlex/#0205(git)_crash_real_path-v1...

Failed to clone: malformed refspec, separators are wrong!

@SyntevoAlex
Copy link
Author

Judging from the log, I think that CirrusCI might stumble over your naming convention using hash symbols and parentheses:

Yes, that's what I wanted to say when I mentioned you. Sorry for being cryptic.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 6, 2020

This branch is now known as am/real-path-fix.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 6, 2020

This patch series was integrated into pu via git@5eec745.

@gitgitgadget gitgitgadget bot added the pu label Mar 6, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented Mar 9, 2020

This patch series was integrated into pu via git@09403e4.

@SyntevoAlex
real_path: remove unsafe API
29e7133 
Returning a shared buffer invites very subtle bugs due to reentrancy or
multi-threading, as demonstrated by the previous patch.

There was an unfinished effort to abolish this [1].

Let's finally rid of `real_path()`, using `strbuf_realpath()` instead.

This patch uses a local `strbuf` for most places where `real_path()` was
previously called.

However, two places return the value of `real_path()` to the caller. For
them, a `static` local `strbuf` was added, effectively pushing the
problem one level higher:
    read_gitfile_gently()
    get_superproject_working_tree()

[1] https://lore.kernel.org/git/1480964316-99305-1-git-send-email-bmwill@google.com/

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
@SyntevoAlex
real_path_if_valid(): remove unsafe API
a491763 
This commit continues the work started with previous commit.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
@SyntevoAlex
get_superproject_working_tree(): return strbuf
4195006 
Together with the previous commits, this commit fully fixes the problem
of using shared buffer for `real_path()` in `get_superproject_working_tree()`.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
@SyntevoAlex
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 10, 2020

Submitted as pull.575.v2.git.1583845884.gitgitgadget@gmail.com

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 10, 2020

This patch series was integrated into pu via git@bb604d3.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 10, 2020

This patch series was integrated into pu via git@3a98500.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 11, 2020

This patch series was integrated into pu via git@eeb9066.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 11, 2020

This patch series was integrated into next via git@1f84382.

@gitgitgadget gitgitgadget bot added the next label Mar 11, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented Mar 12, 2020

This patch series was integrated into pu via git@8904578.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 15, 2020

This patch series was integrated into pu via git@a41c373.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 15, 2020

This patch series was integrated into pu via git@f2eeed7.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 16, 2020

This patch series was integrated into pu via git@d11e8d6.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 18, 2020

This patch series was integrated into pu via git@7b7dc7c.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 18, 2020

This patch series was integrated into pu via git@782e80d.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 20, 2020

This patch series was integrated into pu via git@6880a95.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 21, 2020

This patch series was integrated into pu via git@454338c.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 21, 2020

This patch series was integrated into pu via git@d76d8d1.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 23, 2020

This patch series was integrated into pu via git@e404196.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 25, 2020

This patch series was integrated into pu via git@77b0881.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 25, 2020

This patch series was integrated into pu via git@71c0542.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 25, 2020

This patch series was integrated into pu via git@4d0e899.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 25, 2020

This patch series was integrated into next via git@4d0e899.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 25, 2020

This patch series was integrated into master via git@4d0e899.

@gitgitgadget gitgitgadget bot added the master label Mar 25, 2020
@gitgitgadget gitgitgadget bot closed this Mar 25, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented Mar 25, 2020

Closed via 4d0e899.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gitgitgadget gitgitgadget[bot] gitgitgadget left review comments

Assignees
No one assigned
Labels
master next pu
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@SyntevoAlex @dscho