Bug#904685: diffoscope: RuntimeError when trying to extract an encrypted file (.bmp)

[Chris Lamb]

tags 904685 + patch
thanks

Dear Ricardo,

> Based on this bug, please find attached a proposed patch for handling
> this error gracefully [..]

Wow, thank you so much for this :)

> Additionally, I see that I could have also just submitted a merge request
> via salsa.debian.org. What is the usual workflow, email patches or merge
> requests?

It might depend more on the size of the patch and whether you were
thinking of making more in the future. Using salsa also has the
advantages of running the testsuite too, which may be useful here (see
below). Please do join our group on salsa ...

> I would gladly appreciate some feedback. I tried to update the changelog as
> best as I understood here
> <https://reproducible-builds.org/contribute/#Fixing_issues>.

Sure thing.

So, the debian/changelog entries for diffoscope are generated
automatically upon release so updating the changelog is not required.
In your commit message please suffix with "(Closes: #904685)" so the
versions/state is handled properly though.

(Furthermore, the "~reproducibleX" suffix as outlined on that page is
typically used when we fork packages from /elsewhere/ in Debian and, as
this project is part of the reproducible builds effort, there is no
naturally need to fork..)


           targetpath = os.path.join(dest_dir, os.path.basename(member_name)).encode(
               sys.getfilesystemencoding(), errors='replace')
  -        with self.archive.open(member_name) as source, open(targetpath, 'wb') as target:
  -            shutil.copyfileobj(source, target)
  -        return targetpath.decode(sys.getfilesystemencoding())
  +        try:
  +            with self.archive.open(member_name) as source, open(targetpath, 'wb') as target:
  +                shutil.copyfileobj(source, target)
  +            return targetpath.decode(sys.getfilesystemencoding())
  +        except Exception as exc:
                  ^^^^^^^^^

Isn't this a bit too "wide" an exception class to catch? How narrow can
we safely make it?

  +            raise ContainerExtractionError(member_name, exc)

I think I would also like to see:

 * A comment in the except block explaining why we might be seeing an
   exception in the first place.

 * A test :)


Best wishes,

-- 
      ,''`.
     : :'  :     Chris Lamb
     `. `'`      lamby at debian.org / chris-lamb.co.uk
       `-