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

[Ricardo Gaviria]

Hi Chris,

Thanks for the swift feedback, and for sharing. I have issued a merge
request which can be found here
<https://salsa.debian.org/reproducible-builds/diffoscope/merge_requests/10>.
Have addressed your comments, including narrower exception, accompanied by
a comment, and one test case trying to compare 2 encrypted zipfiles.



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

You are right that is too wide, the narrowest I think we can go is catching
a RuntimeError (which is not that narrow IMO) but that is what is thrown in
this case by the open() method in zipfile.py
<https://github.com/python/cpython/blob/a2fe1e52eb94c41d9ebce1ab284180d7b1faa2a4/Lib/zipfile.py#L1495>

If we want to be more explicit we could just explicitly check if the
archive is encrypted and raise a diffoscope exception accordingly:

+        try:
+            # Wrapped in a try block as exception may be raised due to
archive
+            # being encrypted, already closed, or opened incorrectly see
library
+            # zipfile.py line 1292
+            with self.archive.open(member_name) as source,
open(targetpath, 'wb') as target:
+                shutil.copyfileobj(source, target)
+            return targetpath.decode(sys.getfilesystemencoding())
+        except RuntimeError as exc:
+            raise ContainerExtractionError(member_name, exc)

Not sure what is the best approach here.

We can move this discussion over to the MR if that suits better. Just let
me know.

Thanks again,
Ricardo






On Sun, Aug 5, 2018 at 4:00 AM Chris Lamb <lamby at debian.org> wrote:

> 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
>        `-
>
-- 
Regards,
Ricardo Gaviria
Software Engineer, UniteLabs
*M: *+41 77 956 2376
*W: *http://unitelabs.ch
*In: *https://www.linkedin.com/in/ricardogaviria/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://alioth-lists.debian.net/pipermail/reproducible-builds/attachments/20180805/c299669b/attachment.html>