Hi,

This is via <https://github.com/lamby/trydiffoscope/issues/35>, but I
think the bug is in diffoscope itself.

So, given the following test:

  import os
  import pytest
  import subprocess

  def test_invalid_filename(capsys, tmpdir):
      base = str(tmpdir.mkdir('src')).encode('utf-8')

      a = os.path.join(base, b'\xf0\x28\x8c\x28')
      b = os.path.join(base, b'\xf0\x28\x8c\x29')

      with open(a, 'w'), open(b, 'w'):
          pass

      subprocess.check_call(('bin/diffoscope', a, b))

I get:

Control: tag -1 patch

It is, although one could say it's a bug in argparse.
traceback by using this instead of the last command above:

|>>> m.file(filename.decode('utf-8', errors='surrogateescape'))
|Traceback (most recent call last):
|  File "<stdin>", line 1, in <module>
|  File "/usr/lib/python3/dist-packages/magic/compat.py", line 148, in file
|    return Magic.__tostr(_file(self._magic_t, Magic.__tobytes(filename)))
|  File "/usr/lib/python3/dist-packages/magic/compat.py", line 138, in __tobytes
|    return bytes(b, 'utf-8')
|UnicodeEncodeError: 'utf-8' codec can't encode character '\udcf0' in position 0: surrogates not allowed

What do you think if we try to use:
|>>> m.file(f.encode('utf-8', errors='surrogateescape'))
In that place?

I.e., the following patch would fix this bug for me.
See also:
https://www.python.org/dev/peps/pep-0383/
https://bugs.python.org/issue21416

|diff --git a/diffoscope/comparators/utils/file.py b/diffoscope/comparators/utils/file.py
|index 4fd49ac..0638ef4 100644
|--- a/diffoscope/comparators/utils/file.py
|+++ b/diffoscope/comparators/utils/file.py
|@@ -68,7 +68,7 @@ class File(object, metaclass=abc.ABCMeta):
|             if not hasattr(self, '_mimedb'):
|                 self._mimedb = magic.open(magic.NONE)
|                 self._mimedb.load()
|-            return self._mimedb.file(path)
|+            return self._mimedb.file(path.encode('utf-8', errors='surrogateescape'))
|
|         @classmethod
|         def guess_encoding(self, path):

Do you think this would be fine?

Hi Mattia,

Ahh! Nice spot.

Whilst this works, would it not be better if we could use bytes for
filenames throughout? I mean, AIUI there is no assumption that
filesystems need to have any form of valid encoding whatsoever, let
alone UTF-8.

However, somewhat happy to see this in diffoscope as it certainly
improves the current state of affairs. If you do commit it, please
include my testcase (or something based on it) that I added in:

https://bugs.debian.org/898022#5


Best wishes,

are of different opinion. Check out the PEP I linked in my previous
email: https://www.python.org/dev/peps/pep-0383/

Together with the argparse bug I also linked:
https://bugs.python.org/issue21416 - apparently it's "hard" (more like
impossible?) to get bytes from the CLI...

I believe that, like that bug is showing, we should just specify
    type=os.fsencode    # https://docs.python.org/3/library/os.html#os.fsencode
in the parser.add_argument() calls using a filename (to make sure
argparse doesn't change output), and then re-encode them before passing
them to functions that can't handle surrogate encoded stuff like this
magic module.

Of course.

Hi Mattia,

Ah, thanks I merely skimmed this one and not the the issue. Hey, the Python developers could be wrong — they didn't like the SOURCE_DATE_EPOCH patches!

(Cool, just checking you saw it as, of course, there was no initial
"patch" tag)

Nice work :)


Regards,

Control: clone -1 -2
Control: tag -2 - patch
Control: severity -2 wishlist
Control: retitle -2 diffoscope: should handle the filenames as bytes

This indeed makes the filename be a .bytes through all the code, and
therefore requires a bunch of changes pretty much everywhere in the
comparators (str.endsiwth → bytes.endswith and with them all the
comparing strings needs to be become bytes as well).

I'm cloning this bug to keep track of this thing, as I'm not going to do
that now.
Initial patch to start (from there just try run it and it will crash)
  |--- a/diffoscope/main.py
  |+++ b/diffoscope/main.py
  |@@ -74,11 +74,13 @@ def create_parser():
  |     parser = argparse.ArgumentParser(
  |         description='Calculate differences between two files or directories',
  |         add_help=False, formatter_class=HelpFormatter)
  |-    parser.add_argument('path1', nargs='?', help='First file or directory to '
  |-                        'compare. If omitted, tries to read a diffoscope diff from stdin.')
  |-    parser.add_argument('path2', nargs='?', help='Second file or directory to '
  |-                        'compare. If omitted, no comparison is done but instead we read a '
  |-                        'diffoscope diff from path1 and will output this in the formats '
  |+    parser.add_argument('path1', nargs='?', type=os.fsencode,
  |+                        help='First file or directory to compare. If omitted, '
  |+                        'tries to read a diffoscope diff from stdin.')
  |+    parser.add_argument('path2', nargs='?', type=os.fsencode,
  |+                        help='Second file or directory to compare. If omitted, '
  |+                        'no comparison is done but instead we read a diffoscope '
  |+                        'diff from path1 and will output this in the formats '
  |                         'specified by the rest of the command line.')
  |     parser.add_argument('--debug', action='store_true',
  |                         default=False, help='Display debug messages')
  |


Also, I believe doing that change also requires changing the handling of
the filenames in the Container objects, thanks to diffoscope's
recursivity.  So really, a quite big change.


In the meantime to fix #898022 I'll apply the patch I posted earlier.

forwarded 898361 https://salsa.debian.org/reproducible-builds/diffoscope/issues/32
thanks

I've forwarded this upstream here:

https://salsa.debian.org/reproducible-builds/diffoscope/issues/32


Regards,