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

Pass unicode string to path.is_path_inside_base_dir #1345

Conversation

@blueyed
Copy link
Contributor

@blueyed blueyed commented Dec 1, 2015

is_path_inside_base_dir expects a unicode string, while the strings
from os.listdir are 8-bit strings (because local_path is one also)
[1].

This fixes the following error:

ERROR    FileBackend backend caused an exception.
Traceback (most recent call last):
  File "…/mopidy/mopidy/core/library.py", line 19, in _backend_error_handling
    yield
  File "…/mopidy/mopidy/core/library.py", line 112, in _browse
    result = backend.library.browse(uri).get()
  File "/usr/lib/python2.7/site-packages/pykka/future.py", line 299, in get
    exec('raise exc_info[0], exc_info[1], exc_info[2]')
  File "/usr/lib/python2.7/site-packages/pykka/actor.py", line 200, in _actor_loop
    response = self._handle_receive(message)
  File "/usr/lib/python2.7/site-packages/pykka/actor.py", line 294, in _handle_receive
    return callee(*message['args'], **message['kwargs'])
  File "…/mopidy/mopidy/file/library.py", line 67, in browse
    if not self._is_in_basedir(os.path.realpath(child_path)):
  File "…/mopidy/mopidy/file/library.py", line 136, in _is_in_basedir
    for media_dir in self._media_dirs)
  File "…/mopidy/mopidy/file/library.py", line 136, in <genexpr>
    for media_dir in self._media_dirs)
  File "…/mopidy/mopidy/internal/path.py", line 213, in is_path_inside_base_dir
    common_prefix = os.path.commonprefix([real_base_path, real_path])
  File "…/pyenv/mopidy/lib/python2.7/genericpath.py", line 79, in commonprefix
    s1 = min(m)
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 15: ordinal not in range(128)

1: https://docs.python.org/2/howto/unicode.html#unicode-filenames

@adamcik
Copy link
Member

@adamcik adamcik commented Dec 5, 2015

Typically we've taken the stance that paths should be bytestrings, as we can never be 100% sure about the FS encodings. Best we can get is the system default encoding. So not sure if this perhaps should be flipped the other way around.

@jodal thoughts?

Loading

@jodal
Copy link
Member

@jodal jodal commented Dec 9, 2015

From the Python doc's Unicode HOWTO (linked above), on Unicode filenames:

Note that on most occasions, the Unicode APIs should be used. The bytes APIs should only be used on systems where undecodable file names can be present, i.e. Unix systems.

So, it seems we've made the right choice for maximum compatibility.

Loading

`is_path_inside_base_dir` expects a unicode string, while the strings
from `os.listdir` are 8-bit strings (because `local_path` is one also)
[1].

This fixes the following error:

```
ERROR    FileBackend backend caused an exception.
Traceback (most recent call last):
  File "…/mopidy/mopidy/core/library.py", line 19, in _backend_error_handling
    yield
  File "…/mopidy/mopidy/core/library.py", line 112, in _browse
    result = backend.library.browse(uri).get()
  File "/usr/lib/python2.7/site-packages/pykka/future.py", line 299, in get
    exec('raise exc_info[0], exc_info[1], exc_info[2]')
  File "/usr/lib/python2.7/site-packages/pykka/actor.py", line 200, in _actor_loop
    response = self._handle_receive(message)
  File "/usr/lib/python2.7/site-packages/pykka/actor.py", line 294, in _handle_receive
    return callee(*message['args'], **message['kwargs'])
  File "…/mopidy/mopidy/file/library.py", line 67, in browse
    if not self._is_in_basedir(os.path.realpath(child_path)):
  File "…/mopidy/mopidy/file/library.py", line 136, in _is_in_basedir
    for media_dir in self._media_dirs)
  File "…/mopidy/mopidy/file/library.py", line 136, in <genexpr>
    for media_dir in self._media_dirs)
  File "…/mopidy/mopidy/internal/path.py", line 213, in is_path_inside_base_dir
    common_prefix = os.path.commonprefix([real_base_path, real_path])
  File "…/pyenv/mopidy/lib/python2.7/genericpath.py", line 79, in commonprefix
    s1 = min(m)
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 15: ordinal not in range(128)
```

1: https://docs.python.org/2/howto/unicode.html#unicode-filenames
@blueyed blueyed force-pushed the fix-UnicodeDecodeError-in-is_path_inside_base_dir branch from e6d0ff6 to aeb52a4 Dec 31, 2015
@blueyed
Copy link
Contributor Author

@blueyed blueyed commented Dec 31, 2015

Rebased on develop.

From the previous comments it seems to be the right way to fix this?!

Loading

@jodal jodal added this to the v2.0.1 - Bug fixes milestone Mar 26, 2016
@jodal jodal self-assigned this Mar 26, 2016
jodal added a commit to jodal/mopidy that referenced this issue Mar 26, 2016
@jodal
Copy link
Member

@jodal jodal commented Mar 26, 2016

I'm sorry I haven't gotten back to you on this for three months.

So far we've kept to the policy of paths being bytestrings and URIs being Unicode strings where non-ASCII bytes are URL-encoded, so they are fully preserved. Thus, the preferred solution here is to make sure both paths that are being compared are bytestrings.

(Related note: With our upcoming transition to Python 2+3 compatability, we'll probably switch to using Unicode strings for paths, throwing away support for file systems with varying file name encodings, but making interop with Python 3 APIs far easier for us.)

I've opened PR #1493 to fix the crash you experienced.

Loading

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

Successfully merging this pull request may close these issues.

None yet

3 participants