Disallow " characters in paths

[GoogleCodeExporter]

I get a directory path error, when I try to import track 1 of this release: 
http://musicbrainz.org/release/3ca1335e-54d4-4f79-928b-a3f12bdbed00

The directory does not exist before running the import, and it's actually 
created after giving this error. I'm running Ubuntu 11.10, and the filesystem 
is ext4.

Here's the traceback:

  File "/usr/local/bin/beet", line 9, in <module>
    load_entry_point('beets==1.0b11', 'console_scripts', 'beet')()
  File "/usr/local/lib/python2.7/dist-packages/beets-1.0b11-py2.7.egg/beets/ui/__init__.py", line 627, in main
    subcommand.func(lib, config, suboptions, subargs)
  File "/usr/local/lib/python2.7/dist-packages/beets-1.0b11-py2.7.egg/beets/ui/commands.py", line 673, in import_func
    timid, query, incremental, ignore)
  File "/usr/local/lib/python2.7/dist-packages/beets-1.0b11-py2.7.egg/beets/ui/commands.py", line 572, in import_files
    ignore = ignore,
  File "/usr/local/lib/python2.7/dist-packages/beets-1.0b11-py2.7.egg/beets/importer.py", line 806, in run_import
    pl.run_parallel(QUEUE_SIZE)
  File "/usr/local/lib/python2.7/dist-packages/beets-1.0b11-py2.7.egg/beets/util/pipeline.py", line 242, in run
    out = self.coro.send(msg)
  File "/usr/local/lib/python2.7/dist-packages/beets-1.0b11-py2.7.egg/beets/importer.py", line 632, in apply_choices
    lib.move(item, do_copy, task.is_album)
  File "/usr/local/lib/python2.7/dist-packages/beets-1.0b11-py2.7.egg/beets/library.py", line 969, in move
    item.move(dest, copy)
  File "/usr/local/lib/python2.7/dist-packages/beets-1.0b11-py2.7.egg/beets/library.py", line 219, in move
    util.copy(self.path, dest)
  File "/usr/local/lib/python2.7/dist-packages/beets-1.0b11-py2.7.egg/beets/util/__init__.py", line 234, in copy
    return shutil.copyfile(path, dest)
  File "/usr/lib/python2.7/shutil.py", line 82, in copyfile
    with open(dst, 'wb') as fdst:
IOError: [Errno 2] No such file or directory: '/var/storage/mount/music/Duke 
Ellington & His Orchestra/Hot Summer Dance/01 Take the "A" Train.flac'

Original issue reported on code.google.com by pe...@e-pedro.com on 14 Nov 2011 at 10:01

[GoogleCodeExporter]

This is another weird one. In my tests, I can't make this occur, no matter how 
much I include &s and "s in the filename. I'm testing on ext3 under Arch Linux, 
but that shouldn't make a difference.

Can you run a test on this filename in isolation? I.e., open a Python prompt 
and type something like:
>>> open('/var/storage/mount/music/Duke Ellington & His Orchestra/Hot Summer 
Dance/01 Take the "A" Train.flac', 'wb')

See if that throws the same exception. If so, we'll know your system doesn't 
like that filename.

Original comment by adrian.sampson on 18 Nov 2011 at 4:41

• Changed state: Accepted

[GoogleCodeExporter]

That's the thing. The file is not copied. When I get that error, only the 
destination directory is created.

Original comment by pe...@e-pedro.com on 19 Nov 2011 at 11:43

[GoogleCodeExporter]

Right, that open() call will try *creating* the file (simulating the copy 
operation).

Original comment by adrian.sampson on 19 Nov 2011 at 11:53

[GoogleCodeExporter]

Oh. My bad. It throws the same exception.

Original comment by pe...@e-pedro.com on 20 Nov 2011 at 12:16

[GoogleCodeExporter]

If I remove the quotes around the A it works.

Original comment by pe...@e-pedro.com on 20 Nov 2011 at 12:16

[GoogleCodeExporter]

Thanks! Oddly, I still can't make any of my Linux boxes complain when creating 
files with " in them, but for now I think the safest thing to do is add that 
character to the substitution blacklist.

Original comment by adrian.sampson on 20 Nov 2011 at 2:11

• Changed title: Disallow " characters in paths
• Added labels: Type-Defect, Priority-High, Milestone-1.0b11, Component-Library

[GoogleCodeExporter]

Hi!  Was just browsing around the project and saw this, thought I'd add in 
something (hopefully) helpful:

While it might not be the best idea to have files with strange characters in 
them, it's perfectly legal for all of the characters above, and I don't think 
that's the problem.  I.e. open from the above stack trace will not fail because 
of those characters.

More likely there's a shell command earlier on where it's passed as an argument 
without being properly escaped (i.e. mkdir -p /path/to/strange & unusual 
characters/).  I wouldn't be surprised to find a directory 
'/var/storage/mount/music/Duke Ellington ' and an error about "Hot Summer 
Dance/...": command not found, or errors about strange named commands that are 
not found, etc.

While as a design decision you might decide that you don't want to (or can't, 
like '/') create files with certain characters in their name, you still need to 
make sure you're properly handling/quoting/escaping them, because they might be 
coming from an external and untrusted source.  

For example, if you ripped the l33t h4x0rz song "I like to `rm -rf /`", even if 
you don't keep "`" in the output filename it sounds to me like it's being given 
to a shell command somewhere in the meantime, perhaps for re-tagging or 
similar, which would not be super fun :)

Original comment by sean...@gmail.com on 21 Nov 2011 at 1:24

[GoogleCodeExporter]

and btw, proper escaping, assuming the command is a single string being passed 
to os.system() or similar, would be to take each argument, replace every 
instance of a single quote with an escaped single quote surrounded by two more 
single quotes ('\''), and then single quote the entire resulting value.  i.e., 
Daft Punk's "Rollin' & Scratchin'" would become 'Rollin'\'' & Scratchin'\'''.

hope that helps!

Original comment by sean...@gmail.com on 21 Nov 2011 at 1:34

[GoogleCodeExporter]

Thanks for the comments, seanius, but in this case these paths are never being 
passed to a shell. The Python open() invocation we experimented with above 
never goes through any shell; it invokes the UNIX system call directly.

Original comment by adrian.sampson on 22 Nov 2011 at 7:23

[GoogleCodeExporter]

Character added to blacklist in 9d6057fef332.

Original comment by adrian.sampson on 26 Nov 2011 at 10:38

• Changed state: Fixed