Grid.decode() returns InvalidVersion for valid QR codes with high versions

[danya02]

For instance, this example from Wikipedia is a perfectly cromulent QR code, as evidenced by other QR decoders such as this one.

Here's a copy of it in GIF format:

However, if you try to decode it with this library, you get an error:

use rqrr;
use image;

fn main() {
    let img = match image::open("ver10.gif") {Ok(img)=>img, Err(err)=>panic!("unknown error while decoding img: {:?}", err)};
    let luma = img.to_luma();
    let mut prep_img = rqrr::PreparedImage::prepare(luma);
    let grids = prep_img.detect_grids();
    let res = grids[0].decode();
    let error = res.expect_err("This should succeed, but fails");
    match error {
        rqrr::DeQRError::InvalidVersion => (),
        _ => panic!("unexpected error in decoding?!"),
    }
}

(Sidenote: you might want to #[derive(PartialEq)] on rqrr::DeQRError -- the match construction is rather unwieldy compared to a simple assert_eq!, and the matches! macro is not yet stable.)

As far as I can tell, this happens because this error gets returned when the width of a QR code is greater than 40, not its version. The extreme case, version 40, has 177 modules per side, which is more than 40, but it doesn't mean that the grid itself is invalid. I've cloned the repo and changed this condition, and the rest of the code worked correctly in the version 10 case, so I'm pretty sure this is the one place that needs to be changed.

[WanzenBug]

Hey!

Thanks for the detailed error report with example and also suspect code. You were totally right, this check was broken. I replaced the version computation with a better version which should cover all defined versions. I also add some more derive clauses for DeQRError.

The fix is part of the newly released version v0.2.1.

[danya02]

Thanks!

By the way, how about adding a test where a grid looks like a QR code, but is actually a wrong version? I just tried it, and it panics with this traceback:

thread 'main' panicked at 'index out of bounds: the len is 41 but the index is 41', /home/danya/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/rqrr-0.2.0/src/identify/grid.rs:349:17
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:77
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:61
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1028
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1412
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:65
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:50
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:188
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:205
  10: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:464
  11: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:373
  12: rust_begin_unwind
             at src/libstd/panicking.rs:302
  13: core::panicking::panic_fmt
             at src/libcore/panicking.rs:139
  14: core::panicking::panic_bounds_check
             at src/libcore/panicking.rs:96
  15: rqrr::identify::grid::fitness_all
             at /home/danya/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/rqrr-0.2.0/src/identify/grid.rs:349
  16: rqrr::identify::grid::jiggle_perspective
             at /home/danya/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/rqrr-0.2.0/src/identify/grid.rs:304
  17: rqrr::identify::grid::setup_perspective
             at /home/danya/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/rqrr-0.2.0/src/identify/grid.rs:152
  18: rqrr::identify::grid::SkewedGridLocation::from_group
             at /home/danya/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/rqrr-0.2.0/src/identify/grid.rs:104
  19: rqrr::prepare::PreparedImage<S>::detect_grids::{{closure}}
             at /home/danya/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/rqrr-0.2.0/src/prepare.rs:213
  20: core::ops::function::impls::<impl core::ops::function::FnMut<A> for &mut F>::call_mut
             at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libcore/ops/function.rs:265
  21: core::iter::traits::iterator::Iterator::find_map::check::{{closure}}
             at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libcore/iter/traits/iterator.rs:2024
  22: core::iter::traits::iterator::Iterator::try_fold
             at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libcore/iter/traits/iterator.rs:1709
  23: core::iter::traits::iterator::Iterator::find_map
             at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libcore/iter/traits/iterator.rs:2030
  24: <core::iter::adapters::FilterMap<I,F> as core::iter::traits::iterator::Iterator>::next
             at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libcore/iter/adapters/mod.rs:966
  25: <alloc::vec::Vec<T> as alloc::vec::SpecExtend<T,I>>::from_iter
             at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/liballoc/vec.rs:1988
  26: <alloc::vec::Vec<T> as core::iter::traits::collect::FromIterator<T>>::from_iter
             at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/liballoc/vec.rs:1901
  27: core::iter::traits::iterator::Iterator::collect
             at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libcore/iter/traits/iterator.rs:1493
  28: rqrr::prepare::PreparedImage<S>::detect_grids
             at /home/danya/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/rqrr-0.2.0/src/prepare.rs:212
  29: qrtest::main
             at src/main.rs:8
  30: std::rt::lang_start::{{closure}}
             at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libstd/rt.rs:61
  31: std::rt::lang_start_internal::{{closure}}
             at src/libstd/rt.rs:48
  32: std::panicking::try::do_call
             at src/libstd/panicking.rs:287
  33: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:78
  34: std::panicking::try
             at src/libstd/panicking.rs:265
  35: std::panic::catch_unwind
             at src/libstd/panic.rs:396
  36: std::rt::lang_start_internal
             at src/libstd/rt.rs:47
  37: std::rt::lang_start
             at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libstd/rt.rs:61
  38: main
  39: __libc_start_main
  40: _start
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

To reproduce, use this grid I made that looks like a QR code but isn't:

Here's the GIMP file I created this from. Essentially what I've done is added 4 modules of width to the QR code, thus making it a version 41 code, which doesn't exist. This sounds like a good time to emit DeQRError::InvalidVersion, rather than to panic.

[danya02]

By the way, it seems that not many QR decoders can gracefully recover from parsing this grid. My Xiaomi-branded phone has a builtin QR-scanner app that interprets this code as a string of 8 random-ish numbers.

Meanwhile, that decoder I linked returned this error (line breaks added):

Fatal error: Uncaught InvalidArgumentException in /home/g2goipecfhbt/public_html/elqr/vendor/khanamiryan/qrcode-detector-decoder/lib/Qrcode/Decoder/Version.php:121
Stack trace:
#0 /home/g2goipecfhbt/public_html/elqr/vendor/khanamiryan/qrcode-detector-decoder/lib/Qrcode/Decoder/Version.php(112): Zxing\Qrcode\Decoder\Version::getVersionForNumber(41)
#1 /home/g2goipecfhbt/public_html/elqr/vendor/khanamiryan/qrcode-detector-decoder/lib/Qrcode/Detector/Detector.php(84): Zxing\Qrcode\Decoder\Version::getProvisionalVersionForDimension(181)
#2 /home/g2goipecfhbt/public_html/elqr/vendor/khanamiryan/qrcode-detector-decoder/lib/Qrcode/Detector/Detector.php(69): Zxing\Qrcode\Detector\Detector->processFinderPatternInfo(Object(Zxing\Qrcode\Detector\FinderPatternInfo))
#3 /home/g2goipecfhbt/public_html/elqr/vendor/khanamiryan/qrcode-detector-decoder/lib/Qrcode/QRCodeReader.php(62): Zxing\Qrcode\Detector\Detector->detect(NULL)
#4 /home/g2goipecfhbt/public_html/elqr/vendor/khanamiryan/qrcode-detector-decoder/lib/QrReader.php(80): Zxing\Qr in /home/g2goipecfhbt/public_html/elqr/vendor/khanamiryan/qrcode-detector-decoder/lib/Qrcode/Decoder/Version.php on line 121

So, this may be too much to ask for to gracefully handle this case if the venerable ZXing project was unable to.

[WanzenBug]

Actually I had a check with version v0.2.1 and it already returns InvalidVersion

This test passes:

let gif = image::open("tests/data/full/invalidversion.gif").unwrap().to_luma();

let mut search_img = rqrr::PreparedImage::prepare(gif);
let grids = search_img.detect_grids();
assert_eq!(grids.len(), 1);
let decoded = grids[0].decode();
assert!(decoded.is_err());
let err = decoded.unwrap_err();
assert_eq!(err, rqrr::DeQRError::InvalidVersion);

I used the gif you linked above

[danya02]

All right, I might have been testing with an older version of the library. Still, it seems like it'd be a good idea to add a test for that case -- it just feels prudent to check the error conditions in addition to normal cases, especially when, as in this case, errors in error handling can lead to crashes on maliciously-constructed grids.

If you'd like, I can try and invent some more test cases involving errors and package them up into a PR. In particular I'm interested in the DataEcc and FormatEcc cases, because they seem to be most likely to arise in normal usage.

[WanzenBug]

Contributions are always welcome 😃