TagStudioDev / TagStudio

A User-Focused Photo & File Management System
GNU General Public License v3.0
2.85k stars 266 forks source link

Fix pillow decompression bomb error mentioned in #164 #166

Closed Thesacraft closed 1 month ago

Thesacraft commented 1 month ago

Prevents DecompressionBombError that is triggered by pictures with to many pixels. If the picture is to big it prevents the thumbnail from being rendered but allows the user to tag the file etc. (Just without a thumbnail) This should fix Issue #164

yedpodtrzitko commented 1 month ago

This code is in the codebase in one more place, so it would be worth fix it there as well, and add the missing the exif_transpose() there

https://github.com/TagStudioDev/TagStudio/blob/3aa71d6f8abb67f309f6d66ef4e6c570332d82be/tagstudio/src/qt/widgets/preview_panel.py#L371-L377

Also it's a bit out of scope, but when you are touching this code already, it would be worth it to DRY it and extract it into a function instead of having exactly the same block of code in three different places.

Thesacraft commented 1 month ago

Ohh i didn't see that one thank you! I will look into moving this code into its own function tommorow and all other changes(im currently procastinating and have a final exam tomorrow hahaha). Just one more question: Do you know why in the preview_panel only the Video and image types are handled (line 368 onwards) and only in the Thumbrenderer handles text types?

yedpodtrzitko commented 1 month ago

Do you know why in the preview_panel only the Video and image types are handled (line 368 onwards) and only in the Thumbrenderer handles text types?

no idea, maybe person who was adding the code for text files overlooked the logic in preview panel, similarly as it happened here? :)

CyanVoxel commented 1 month ago

Do you know why in the preview_panel only the Video and image types are handled (line 368 onwards) and only in the Thumbrenderer handles text types?

no idea, maybe person who was adding the code for text files overlooked the logic in preview panel, similarly as it happened here? :)

The foreboding line of 366: # TODO: Do this somewhere else, this is just here temporarily.

Now if I were to put myself in the mindset of whoever may have written this code... It looks like the image/video frames were being loaded here just to grab their dimensions to display in the preview panel. Absolutely not the right place to do this, as 366 warned, but still it's remained there. I bet whoever wrote this was planning on displaying specs from other filetypes like word count or line length for text files, video length for videos, and things like that - but knew that allll off that would be better off in a different class which preview_panel could just call from.

That's just my speculation though 😉

Thesacraft commented 1 month ago

This code is in the codebase in one more place, so it would be worth fix it there as well, and add the missing the exif_transpose() there

https://github.com/TagStudioDev/TagStudio/blob/3aa71d6f8abb67f309f6d66ef4e6c570332d82be/tagstudio/src/qt/widgets/preview_panel.py#L371-L377

Also it's a bit out of scope, but when you are touching this code already, it would be worth it to DRY it and extract it into a function instead of having exactly the same block of code in three different places.

I intentionally did not DRY it because i'm thinking about a way for it to be moved completly out of there. I DRYed the other two occurances in PR #167 (just to seperate things properly), or would you prefer that the DecompressionBombError is also just fixed in #167 and this one is closed?

CyanVoxel commented 1 month ago

I intentionally did not DRY it because i'm thinking about a way for it to be moved completly out of there. I DRYed the other two occurances in PR #167 (just to seperate things properly), or would you prefer that the DecompressionBombError is also just fixed in #167 and this one is closed?

I would suggest using this current PR to address the DecompressionBombError in #164, which I'll pull first. Then I'll work with you on #167 to substantially DRY this class in a way that will prepare it for future functionality and make it simpler to interface with - if you're alright with that ;)

Thesacraft commented 1 month ago

I think i handled every DecompressionBombError that were occuring