image icon indicating copy to clipboard operation
image copied to clipboard

Add "safe" opening of images

Open qarmin opened this issue 3 years ago • 1 comments

Currently when opening images, sometimes library crashes due bugs that are fixed in master branch(I can only use stable releases from crates.io) or not fixed yet.

Before I just ignored files with specific extensions that cause crashes, but this was problematic because a lot of proper files was not used.

In https://github.com/qarmin/czkawka/commit/de964ce40d704330fbb45a5194607364fa251cb5 I added in my app ability to run image opening code in closure, so I can catch and handle there panic.

This probably have some performance impact and require to use panic="unwind" in Cargo.toml

I think that such functionality should be integrated directly in this crate by adding new method that will wrap wrap result from open function in another result type

This is how code changed in my implementation before

                        match image::open(&file_entry.path) {
                            Ok(_) => Some(None),
                            Err(t) => {
                                let error_string = t.to_string();
                                // This error is a problem with image library, remove check when https://github.com/image-rs/jpeg-decoder/issues/130 will be fixed
                                if !error_string.contains("spectral selection is not allowed in non-progressive scan") {
                                    let mut file_entry = file_entry.clone();
                                    file_entry.error_string = error_string;
                                    Some(Some(file_entry))
                                } else {
                                    Some(None)
                                }
                            } // Something is wrong with image
                        }

after

                        let result = panic::catch_unwind(|| {
                            match image::open(&file_entry.path) {
                                Ok(_) => Some(None),
                                Err(t) => {
                                    let error_string = t.to_string();
                                    // This error is a problem with image library, remove check when https://github.com/image-rs/jpeg-decoder/issues/130 will be fixed
                                    if !error_string.contains("spectral selection is not allowed in non-progressive scan") {
                                        file_entry.error_string = error_string;
                                        Some(Some(file_entry))
                                    } else {
                                        Some(None)
                                    }
                                }
                            }
                        });

                        // If image crashed during opening, we just skip checking its hash and go on
                        if let Ok(image_result) = result {
                             image_result
                        } else {
                            println!("Image-rs library crashed when opening \"{:?}\" image, please check if problem happens with latest image-rs version(this can be checked via https://github.com/qarmin/ImageOpening tool) and if it is not reported, please report bug here - https://github.com/image-rs/image/issues", file_entry_clone.path);
                             Some(Some(file_entry_clone))
                        }

qarmin avatar Dec 30 '21 07:12 qarmin

By design we can not catch double-panics which instantly abort, nor protect against malicious resource usage, so this would be seriously misnamed. I'm 100% on board with adding this as a usage example/pattern to the documentation anyways! However, without actionable reports that generate images we can use for diagnosing the error and regression testing I'm afraid I do not see as much value in adding the function itself as you apparently do.

197g avatar Jan 12 '22 17:01 197g