Fix panic in color parsing #290

smackysnacks · 2024-05-29T15:25:20Z

Input:

⇒ hexdump -C fuzz/artifacts/tiled/badcolor 
00000000  3c 3f 78 6d 6c 20 76 65  72 73 69 6f 6e 3d 22 31  |<?xml version="1|
00000010  2e 30 22 20 65 6e 63 6f  64 69 6e 67 3d 22 55 54  |.0" encoding="UT|
00000020  46 2d 38 22 3f 3e 0d 3c  6d 61 70 20 76 65 6e 66  |F-8"?>.<map venf|
00000030  69 6e 65 3d 22 30 22 20  62 61 63 6b 67 72 6f 75  |ine="0" backgrou|
00000040  6e 64 63 6f 6c 6f 72 3d  22 23 66 66 30 d0 92 66  |ndcolor="#ff0..f|
00000050  22 20 6e 63 74 69 64 3d  22 35 22 3e              |" nctid="5">|
0000005c

Running: fuzz/artifacts/tiled/minimized-from-5b11a24d56d555d9230c3aa06adbfd12b6a00a89
thread '<unnamed>' panicked at /mnt/e/code/rs-tiled/src/properties.rs:32:46:
byte index 4 is not a char boundary; it is inside 'В' (bytes 3..5) of `ff0Вf`
stack backtrace:
   0:     0x563f2dc9d095 - std::backtrace_rs::backtrace::libunwind::trace::h9fff41df29930226
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/std/src/../../backtrace/src/backtrace/libunwind.rs:105:5
   1:     0x563f2dc9d095 - std::backtrace_rs::backtrace::trace_unsynchronized::hd333ee9fabe37696
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
   2:     0x563f2dc9d095 - std::sys_common::backtrace::_print_fmt::ha8cea204a14d4b05
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/std/src/sys_common/backtrace.rs:68:5
   3:     0x563f2dc9d095 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h23b6b3b597037ccc
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/std/src/sys_common/backtrace.rs:44:22
   4:     0x563f2dcec11b - core::fmt::rt::Argument::fmt::h108a26a03f438748
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/core/src/fmt/rt.rs:165:63
   5:     0x563f2dcec11b - core::fmt::write::h04064cb45a345462
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/core/src/fmt/mod.rs:1172:21
   6:     0x563f2dc91e9f - std::io::Write::write_fmt::h222f9a473033d72e
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/std/src/io/mod.rs:1835:15
   7:     0x563f2dc9ce6e - std::sys_common::backtrace::_print::h0d72338fa5455ac9
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/std/src/sys_common/backtrace.rs:47:5
   8:     0x563f2dc9ce6e - std::sys_common::backtrace::print::he02582a61c39a81d
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/std/src/sys_common/backtrace.rs:34:9
   9:     0x563f2dc9f899 - std::panicking::default_hook::{{closure}}::h93f8f6e01b4e4fd9
  10:     0x563f2dc9f63a - std::panicking::default_hook::hbc1b8395cdf679f0
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/std/src/panicking.rs:298:9
  11:     0x563f2dbaeb10 - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call::h0913f36c163d89aa
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/alloc/src/boxed.rs:2077:9
  12:     0x563f2dbb1507 - libfuzzer_sys::initialize::{{closure}}::hc58cb4b6be71cc7d
                               at /home/smacks/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libfuzzer-sys-0.4.7/src/lib.rs:90:9
  13:     0x563f2dc9ffcb - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call::h79ea62f4492f4aab
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/alloc/src/boxed.rs:2077:9
  14:     0x563f2dc9ffcb - std::panicking::rust_panic_with_hook::h2d3b3b5d41eafa75
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/std/src/panicking.rs:799:13
  15:     0x563f2dc9fd44 - std::panicking::begin_panic_handler::{{closure}}::hc78e06bc8800937b
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/std/src/panicking.rs:664:13
  16:     0x563f2dc9d559 - std::sys_common::backtrace::__rust_end_short_backtrace::h10081666adae5ac0
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/std/src/sys_common/backtrace.rs:171:18
  17:     0x563f2dc9fa77 - rust_begin_unwind
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/std/src/panicking.rs:652:5
  18:     0x563f2dce86e3 - core::panicking::panic_fmt::he2bec51da94b7c97
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/core/src/panicking.rs:72:14
  19:     0x563f2dcf091f - core::str::slice_error_fail_rt::h6bddaaeee498b4b6
  20:     0x563f2dcf04ca - core::str::slice_error_fail::hba1d992429648999
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/core/src/str/mod.rs:89:5
  21:     0x563f2d7835af - core::str::traits::<impl core::slice::index::SliceIndex<str> for core::ops::range::Range<usize>>::index::h394ff7554f857129
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/core/src/str/traits.rs:244:21
  22:     0x563f2d7bb639 - core::str::traits::<impl core::ops::index::Index<I> for str>::index::h2c1445a2bf398d68
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/core/src/str/traits.rs:62:9
  23:     0x563f2d7500f3 - <tiled::properties::Color as core::str::traits::FromStr>::from_str::hdbbb9442ee3a1c29
                               at /mnt/e/code/rs-tiled/src/properties.rs:32:46
  24:     0x563f2d63e95d - core::str::<impl str>::parse::hd1cc564dd1effe4c
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/core/src/str/mod.rs:2425:9
  25:     0x563f2d6825c7 - tiled::map::Map::parse_xml::h1d113b9054255d38
                               at /mnt/e/code/rs-tiled/src/map.rs:140:54
  26:     0x563f2d62f7ad - tiled::parse::xml::map::parse_map::h28ea4b43dc040a33
                               at /mnt/e/code/rs-tiled/src/parse/xml/map.rs:27:28
  27:     0x563f2d5b86f5 - tiled::loader::Loader<Cache,Reader>::load_tmx_map::h123d31838d954b75
                               at /mnt/e/code/rs-tiled/src/loader.rs:169:9
  28:     0x563f2d6d13d1 - tiled::_::__libfuzzer_sys_run::he5c7aa3d48cb477e
                               at /mnt/e/code/rs-tiled/fuzz/fuzz_targets/tiled.rs:31:13
  29:     0x563f2d6d0e16 - rust_fuzzer_test_input
                               at /home/smacks/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libfuzzer-sys-0.4.7/src/lib.rs:224:17
  30:     0x563f2dbb0ce5 - libfuzzer_sys::test_input_wrap::{{closure}}::h92923ccbb00bba0c
                               at /home/smacks/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libfuzzer-sys-0.4.7/src/lib.rs:61:9
  31:     0x563f2dbaad4d - std::panicking::try::do_call::h34f81e05f62ad1f6
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/std/src/panicking.rs:559:40
  32:     0x563f2dbb185b - __rust_try
  33:     0x563f2dbaa9ef - std::panicking::try::h643cc48aebb6fcb6
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/std/src/panicking.rs:523:19
  34:     0x563f2dbaa836 - std::panic::catch_unwind::h3e1fa6f6ac0fe63a
                               at /rustc/36153f1a4e3162f0a143c7b3e468ecb3beb0008e/library/std/src/panic.rs:149:14
  35:     0x563f2dbb09c4 - LLVMFuzzerTestOneInput
                               at /home/smacks/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libfuzzer-sys-0.4.7/src/lib.rs:59:22
  36:     0x563f2dbe5454 - _ZN6fuzzer6Fuzzer15ExecuteCallbackEPKhm
                               at /home/smacks/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libfuzzer-sys-0.4.7/libfuzzer/FuzzerLoop.cpp:612:15
  37:     0x563f2dbb4624 - _ZN6fuzzer10RunOneTestEPNS_6FuzzerEPKcm
                               at /home/smacks/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libfuzzer-sys-0.4.7/libfuzzer/FuzzerDriver.cpp:324:21
  38:     0x563f2dbb90ae - _ZN6fuzzer12FuzzerDriverEPiPPPcPFiPKhmE
                               at /home/smacks/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libfuzzer-sys-0.4.7/libfuzzer/FuzzerDriver.cpp:860:19
  39:     0x563f2dbb18d4 - main
                               at /home/smacks/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libfuzzer-sys-0.4.7/libfuzzer/FuzzerMain.cpp:20:30
  40:     0x7f716c084d90 - <unknown>
  41:     0x7f716c084e40 - __libc_start_main
  42:     0x563f2d4e3925 - _start
  43:                0x0 - <unknown>

Note how the background color here is composed of 6 bytes, but the code before this change was trying to slice in the middle of a char boundary.

aleokdev · 2024-05-29T16:07:03Z

Thanks!! Could you please update the CHANGELOG.md file, and while you're at it also add the other PR we forgot to add there?

aleokdev · 2024-05-29T16:31:48Z

Thank you for all these fixes! It's really satisfying to close these after having implemented fuzzing.

smackysnacks · 2024-05-29T16:55:18Z

Happy to help! This was my first real experience with fuzzing. It'll be a part of my arsenal of tools/techniques from here on out lol

bjorn · 2024-05-29T19:23:53Z

src/properties.rs

@@ -27,7 +27,7 @@ impl FromStr for Color {
            s
        };
        match s.len() {
-            6 => {
+            6 if s.is_ascii() => {


Does this really suffice? The docs for from_str_radix say "This function panics if radix is not in the range from 2 to 36.". I think we want is_ascii_hexdigit?

Edit: Erm, of course radix is always 16, so it won't panic because of that... but I still think we'll want to check for the hexdigits instead?

The radix in this case is the constant value 16 and from_str_radix has its own graceful error handling for invalid inputs (first arg). The guard added in this commit protects against invalid string slicing &s[x..y]. Honestly, it would be nice of there was a set of <int>::from_bstr_radix functions.

Oh alright, I was misunderstanding why this panicked. Apparently it's about invalid UTF-8?

Yeah, Strings and string slices (str) must be valid UTF-8 encoded strings. Iterating over the characters (char; valid UTF-8 encoded character) of a string in Rust is typically done with an s.chars() iterator.

Alright, thanks for the explanation! So indeed a check on ascii suffices here.

fix: panic in color parsing when dealing with multibyte chars

6edacde

smackysnacks force-pushed the fix/color-parse-panic branch from 88916a4 to 6edacde Compare May 29, 2024 16:22

aleokdev approved these changes May 29, 2024

View reviewed changes

aleokdev merged commit fc73d52 into mapeditor:next May 29, 2024
3 checks passed

smackysnacks deleted the fix/color-parse-panic branch May 29, 2024 16:26

bjorn reviewed May 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix panic in color parsing #290

Fix panic in color parsing #290

smackysnacks commented May 29, 2024

aleokdev commented May 29, 2024

aleokdev commented May 29, 2024

smackysnacks commented May 29, 2024

bjorn May 29, 2024 •

edited

Loading

smackysnacks May 29, 2024 •

edited

Loading

bjorn May 29, 2024

smackysnacks May 29, 2024 •

edited

Loading

bjorn May 30, 2024

Fix panic in color parsing #290

Fix panic in color parsing #290

Conversation

smackysnacks commented May 29, 2024

aleokdev commented May 29, 2024

aleokdev commented May 29, 2024

smackysnacks commented May 29, 2024

bjorn May 29, 2024 • edited Loading

Choose a reason for hiding this comment

smackysnacks May 29, 2024 • edited Loading

Choose a reason for hiding this comment

bjorn May 29, 2024

Choose a reason for hiding this comment

smackysnacks May 29, 2024 • edited Loading

Choose a reason for hiding this comment

bjorn May 30, 2024

Choose a reason for hiding this comment

bjorn May 29, 2024 •

edited

Loading

smackysnacks May 29, 2024 •

edited

Loading

smackysnacks May 29, 2024 •

edited

Loading