nerfstudio icon indicating copy to clipboard operation
nerfstudio copied to clipboard

Remove image size limits.

Open Zunhammer opened this issue 1 year ago • 5 comments

I suggest to remove the default image size limits from PIL. This causes training of models in nerfstudio to crash with the error: DecompressionBombError: Image size (340840656 pixels) exceeds limit of 178956970 pixels, could be decompression bomb DOS attack.

I see this is meant to be a security feature, however it prevents the usage of large images. I assume people are using mainly their own images in nerfstudio and don't see this as a security issue. Please let me know what you think about it.

Alternatively, there should be at least a param/option to allow large images.

I remove the image size limit at all occurances in the code where PIL is used, however this might not be necessary. E.g. the tests shouldn't be affected at all but I want to keep it consistent and avoid future issues.

Happy to get your feedback.

Zunhammer avatar Apr 10 '24 08:04 Zunhammer

Thanks for the feedback, I see your point and agree. I'll change the PR later accordingly.

Zunhammer avatar Apr 11 '24 20:04 Zunhammer

@brentyi

As far as I see the value must be set after the import and cannot ne set by a function in another file as suggested. This also means a nerfstudio import woulnd't change any behaviour in other libraries/software. I suggest to globally define the default value for the max pixels (None) and set this one on all files where PIL.Image is imported.

Please let me know if I misunderstand and implemented your suggestion wrong, or alternatively where I should globally define the MAX_IMAGE_PIXELS default.

Thanks

Zunhammer avatar Apr 12 '24 16:04 Zunhammer

This also means a nerfstudio import woulnd't change any behaviour in other libraries/software. I suggest to globally define the default value for the max pixels (None) and set this one on all files where PIL.Image is imported.

Sorry for the delayed response here — is this something you observed empirically? This contradicts my understanding of Python semantics: if you assign Image.MAX_IMAGE_PIXELS = None from anywhere (eg just once), then Image.MAX_IMAGE_PIXELS should resolve to None from everywhere Image is imported.

To clarify my set_pil_image_size_limit() context manager suggestion, the intention was to provide a helper for only locally setting Image.MAX_IMAGE_PIXELS. Then you could write:

with set_pil_image_size_limit(None):
    Image.open(...)

where the MAX_IMAGE_PIXELS constant is reassigned at the start of the with block, then reverted to its original value at the end of the block 🙂

brentyi avatar May 07 '24 07:05 brentyi

Hey Brent,

thanks for the reply. I'll adjust and test again soon. Thanks for the input :)

Zunhammer avatar May 13 '24 13:05 Zunhammer

So, applied your suggestion and it seems working fine. I totally got your idea wrong in the first place, thanks for your support and clarifications. Setting the image size limit only for the "open" calls makes a lot of sense instead of specifying at the top of the file.

Currently the docs fail, I assume this originates from the main branch?

Zunhammer avatar Jun 26 '24 14:06 Zunhammer