This is an archive of the discontinued LLVM Phabricator instance.

Fix git-llvm crashing when trying to remove directory while cleaning
ClosedPublic

Authored by teemperor on Mar 11 2019, 4:06 PM.

Details

Summary

I'm trying to push D59198 but it seems that git-llvm push can't handle the fact
that I add a new directory in the patch:

> git llvm push -n
Pushing 1 commit:
  e7c0a9bd136 Correctly look up declarations in inline namespaces
Traceback (most recent call last):
  File "llvm/utils/git-svn//git-llvm", line 431, in <module>
    args.func(args)
  File "llvm/utils/git-svn//git-llvm", line 385, in cmd_push
    clean_svn(svn_root)
  File "llvm/utils/git-svn//git-llvm", line 201, in clean_svn
    os.remove(os.path.join(svn_repo, filename))
IsADirectoryError: [Errno 21] Is a directory: '.git/llvm-upstream-svn/lldb/trunk/packages/Python/lldbsuite/test/expression_command/inline-namespace'

This patch just uses shutil to delete the directory instead of trying to use os.remove
which only works for files.

Diff Detail

Event Timeline

teemperor created this revision.Mar 11 2019, 4:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2019, 4:06 PM
jlebar accepted this revision.Mar 11 2019, 4:46 PM

Thanks!

This revision is now accepted and ready to land.Mar 11 2019, 4:46 PM

Actually, I wonder if we should have some protections built in here, because e.g. if filename contains "/../../" we could end up rm'ing an arbitrary directory on the machine.

This was true before too, but now it's particularly scary that we're doing rmtree.

We could compute the absolute path to svn_repo and the absolute path to join(svn_repo, filename) and ensure that the first is a prefix of the second?

  • Added safety check that we don't delete directories or files outside the llvm staging dir.
jlebar accepted this revision.Mar 12 2019, 12:21 AM

That is much less scary! Thanks.

This revision was automatically updated to reflect the committed changes.