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.

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

check for executables broken on mounted volumes in docker containers on M1 macs #198

Closed
matyastamas opened this issue May 4, 2021 · 4 comments

Comments

@matyastamas
Copy link

matyastamas commented May 4, 2021

Docker for M1 macs uses gRPC FUSE by default to mount shared volumes which messes up the following chain:

  • access system call from within a docker container incorrectly identifies all shared volume files as executable
  • os.access in python inherits this
  • identify.identify.tag_from_path inherits this

Downstream from this are a lot of pre-commit hooks - e.g. check_executables_have_shebangs:

  • pre_commit.commands.run.Classifier.by_types
  • files given to pre_commit_hooks.check_executables_have_shebangs.main

This likely affects many other hooks that depend on the file type filtering.

The issue has been raised on the docker side, and there is a partial work-around (that I have found to cause docker to freeze frequently to the point of being unusable), but it is unclear when/whether it will ultimately be resolved.

Since many hooks don't install natively on M1 macs yet (often even with rosetta 2), it may not be uncommon for folks to run pre-commit from inside a docker image on a shared volume and encounter this issue.

I noticed that the sys.platform == "win32" branch of the function pre_commit_hooks.check_executables_have_shebangs.check_executables() works correctly on my setup and st_modes on files appear to be set and detected correctly:

import os
from identify.identify import tags_from_path
os.access("README.md", os.X_OK), os.access("scripts/pre-commit-check", os.X_OK)
#  (True, # wrong
#   True)  

tags_from_path("README.md"), tags_from_path("scripts/pre-commit-check")
#  ({'plain-text', 'markdown', 'text', 'file', 'executable'},  # wrong
#   {'bash', 'executable', 'file', 'shell', 'text'})

oct(os.stat("README.md").st_mode), oct(os.stat("scripts/pre-commit-check").st_mode)
#  ('0o100600', '0o100755')  # correct
@asottile
Copy link
Member

asottile commented May 4, 2021

this is a bug in docker for mac and should not be fixed here -- dupe #189

@asottile asottile closed this as completed May 4, 2021
@matyastamas
Copy link
Author

matyastamas commented May 4, 2021

Thanks - fair enough!

It's a moot point in that case, but out of curiosity, @chriskuehl, is there a reason you use os.access to check for executables? In the code for this you have retrieved the st_mode directly above this check and used it to determine whether the file is a directory, but then switch to using os.access just to check if it's executable.

I.e. this line

executable = os.access(path, os.X_OK)

could have been:

executable = bool(mode & 0o111)

or more verbosely:

executable = bool(mode & (stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH))

I'm not super familiar, but access and stat also test different things - the former tests the effective permissions (depending on the current user, particularly if they are root) and the later tests for the file system permissions (see access(2) and stat(2)). From the access(2) page:

[access] answers a slightly different question: "(assuming I'm a setuid binary) can the user who invoked me read/write/execute this file?"

@asottile
Copy link
Member

asottile commented May 4, 2021

@matyastamas if you look at the blame, the stat call is from 2 months ago and the access call is from 4 years ago. access was chosen because it is (1) simpler and (2) more reliable than looking at stat modes

@matyastamas
Copy link
Author

Thanks for the quick responses!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants