Skip to content

Do not run legacy script again when this is the one being executed - #981

Merged
asottile merged 3 commits into
pre-commit:masterfrom
tristan0x:fix/infinite-process-recursion
Mar 30, 2019
Merged

Do not run legacy script again when this is the one being executed#981
asottile merged 3 commits into
pre-commit:masterfrom
tristan0x:fix/infinite-process-recursion

Conversation

@tristan0x

@tristan0x tristan0x commented Mar 27, 2019

Copy link
Copy Markdown
Contributor

This patch fixes an infinite process recursion when .git/hooks/pre-commit.legacy executes itself over and over. I don't know exactly how and why this pre-commit.legacy file was created but it is a copy of .git/hooks/pre-commit

@asottile

asottile commented Mar 27, 2019

Copy link
Copy Markdown
Member

hmm this shouldn't be possible as the legacy script only will get created if it isn't our script (unless there's a bug in that!):

pre-commit/pre_commit/commands/install_uninstall.py

Lines 85 to 87 in da00fa9

# If we have an existing hook, move it to pre-commit.legacy
if os.path.lexists(hook_path) and not is_our_script(hook_path):
os.rename(hook_path, legacy_path)

Do you have a reproduction for how that copy script is getting created?

@tristan0x

tristan0x commented Mar 27, 2019

Copy link
Copy Markdown
Contributor Author

hmm this shouldn't be possible as the legacy script only will get created if it isn't our script (unless there's a bug in that!):

yeah, the is_our_script script looks good to me too.

Do you have a reproduction for how that copy script is getting created?

Unfortunately, I don't. I will ask for more information to the workmate to which it happened, but I am pretty sure he didn't create it himself.

@tristan0x

tristan0x commented Mar 29, 2019

Copy link
Copy Markdown
Contributor Author

pre-commit scripts have ID_HASH = '138fd403232d2ddd5efb44317e38bf03'. We don't know how it happened.

@asottile

asottile commented Mar 29, 2019

Copy link
Copy Markdown
Member

Hmmm well if you can reproduce the situation I'd rather fix the bug than bandaiding over it 🤔

@tristan0x

tristan0x commented Mar 29, 2019

Copy link
Copy Markdown
Contributor Author

I agree a fix would definitely be a plus here, but IMOH you should prevent the fork bomb either way.

cp .git/hooks/pre-commit .git/hooks/pre-commit.legacy

is enough to hang your computer at next commit.

@asottile

asottile commented Mar 29, 2019

Copy link
Copy Markdown
Member

My problem with the current PR is it attempts to fix something which is impossible and would hide a bug if it did exist. If you adjust it to assert not __file__.endswith('.legacy'), __file__ (or equivalent) I'd be much more comfortable with this PR

asottile
asottile approved these changes Mar 30, 2019
View reviewed changes

@asottile asottile left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@asottile
asottile merged commit 03ac3b0 into pre-commit:master Mar 30, 2019
@tristan0x
tristan0x deleted the fix/infinite-process-recursion branch March 31, 2019 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asottile asottile asottile approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tristan0x @asottile