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

fetchart: Creates files with incorrect permissions #4622

Open
rsammelson opened this issue Jan 5, 2023 · 9 comments · May be fixed by #5123
Open

fetchart: Creates files with incorrect permissions #4622

rsammelson opened this issue Jan 5, 2023 · 9 comments · May be fixed by #5123
Labels
bug bugs that are confirmed and actionable

Comments

@rsammelson
Copy link

On linux, running beet fetchart creates files with 600 permissions, rather than 644 (or using the default provided by umask).

Setup

  • OS: Debian 11
  • Python version: 3.9.2
  • beets version: 1.6.0
  • Turning off plugins made problem go away (yes/no): issue with plugin

My configuration (output of beet config) is:

fetchart:                                           
    sources: filesystem coverart
    auto: yes                                       
    minwidth: 0                                     
    maxwidth: 0                                     
    quality: 0                                      
    max_filesize: 0                                 
    enforce_ratio: no                               
    cautious: no                                    
    cover_names:                                    
    - cover                                         
    - front                                         
    - art                                           
    - album                                         
    - folder                                        
    google_key: REDACTED                            
    google_engine: 001442825323518660753:hrh5ch1gjzm 
    fanarttv_key: REDACTED
    lastfm_key: REDACTED                            
    store_source: no                                
    high_resolution: no                             
    deinterlace: no                                 
    cover_format:
@sampsyo
Copy link
Member

sampsyo commented Jan 6, 2023

Hi! Can you please include your full configuration, and perhaps a verbose log from a case where fetchart did this?

@sampsyo sampsyo added the needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature." label Jan 6, 2023
@rsammelson
Copy link
Author

rsammelson commented Jan 9, 2023 •
edited

The issue is cause by the fact that temporary files are created with 600 permissions. In set_art:

beets/beets/library.py

Lines 1340 to 1343 in 6989bce

if copy:
util.copy(path, artdest)
else:
util.move(path, artdest)

So if copy is false, the temporary files are copied over, including their incorrect permissions. The value of copy is from not self.src_removed, here:

beets/beetsplug/fetchart.py

Line 1064 in 6989bce

self._set_art(task.album, candidate, not self.src_removed)

where self.src_removed is:

beets/beetsplug/fetchart.py

Lines 981 to 982 in 6989bce

self.src_removed = (config['import']['delete'].get(bool) or
config['import']['move'].get(bool))

so the issue occurs whenever delete or move are set.

I believe the solution is to not use move for temporary files, I'm not sure how to best implement that.

@sampsyo
Copy link
Member

sampsyo commented Jan 10, 2023

Hmm; I'm not entirely sure I understand the root cause: namely, it seems important to know why the original files (the ones being moved) are created with 0600 permissions. It seems like those should be created with umask defaults?

@rsammelson
Copy link
Author

From the python documentation (https://docs.python.org/3/library/tempfile.html#tempfile.mkstemp)

Creates a temporary file in the most secure manner possible. There are no race conditions in the file’s creation, assuming that the platform properly implements the os.O_EXCL flag for os.open(). The file is readable and writable only by the creating user ID. If the platform uses permission bits to indicate whether a file is executable, the file is executable by no one. The file descriptor is not inherited by child processes.

So this is expected behavior from the library. I see in the util folder:

beets/beets/util/__init__.py

Lines 468 to 475 in 6989bce

def move(path, dest, replace=False):
"""Rename a file. `dest` may not be a directory. If `dest` already
exists, raises an OSError unless `replace` is True. Has no effect if
`path` is the same as `dest`. If the paths are on different
filesystems (or the rename otherwise fails), a copy is attempted
instead, in which case metadata will *not* be preserved. Paths are
translated to system paths.
"""

So here it is implied that removing metadata is bad—which is probably true when moving existing images—but when using new images downloaded by the program, the metadata (from tempfile) should be discarded in favor of the default metadata.

@sampsyo
Copy link
Member

sampsyo commented Jan 11, 2023

Got it; wow; thanks for clarifying!! I did not know this about mkstemp and friends.

Seems like we clearly need to either:

  • Do something different from move's current behavior when moving the images into place, or:
  • Create the temporary files with umask default permissions, instead of mkstemp's default permissions.

@sampsyo sampsyo added bug bugs that are confirmed and actionable and removed needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature." labels Jan 11, 2023
@sampsyo sampsyo changed the title Fetchart creates files with incorrect permissions fetchart: Creates files with incorrect permissions Jan 11, 2023
@rsammelson
Copy link
Author

mkstemp does not have a way to change the permissions as far as I can tell, so I'm guessing that the easiest way to do it is to use copy instead of move for temporary files. I'm not sure what the best way to implement that would be though.

@sampsyo
Copy link
Member

sampsyo commented Jan 16, 2023

That makes sense, although it's a tad unfortunate. There are two things that make that route (leaving the temporary files as they are, and copying them instead of moving):

  • It's a little wasteful, obviously, when the destination is on the same filesystem as the temporary directory.
  • It complicates the case when the source is not /tmp but some existing file on the filesystem. Namely, the fetchart plugin can also pick up files from the filesystem (aside from just downloading images from online sources). So this could complicate the code a bit.

@rsammelson
Copy link
Author

Yeah, the second one is why I couldn't figure out a good way to implement it. It would probably be the best to just reset the permissions, but I haven't found a great way of doing that in python that is cross-platform.

@aereaux
Copy link
Contributor

aereaux commented Feb 16, 2024

Hi, I'm getting this problem when importing music from a zip file. It looks like the temporary files created here (https://github.com/beetbox/beets/blob/master/beets/util/__init__.py#L527) are created with 600 permissions. I was wondering why util.move uses all the code it does instead of using something like shutil.move (https://docs.python.org/3/library/shutil.html#shutil.move)?

@aereaux aereaux linked a pull request Feb 25, 2024 that will close this issue
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug bugs that are confirmed and actionable
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use shutil.move() to move files. aereaux/beets
3 participants
@sampsyo @aereaux @rsammelson