[fuzz-test] Arithmetic overflow occurs while using API Layout::split()

[HeeillWang]

Description

I executed fuzzing to test tui-0.19.0 APIs. This reproduced on ratatui as well. More than one overflow occurs for different input values while running Layout::split(). I'll just list up 3 cases with fuzz inputs for reproduce.

To Reproduce

let rect  = Rect::new(fuzz_arg0, fuzz_arg1, fuzz_arg2, fuzz_arg3);
let vchunks  = Layout::default().direction(Direction::Vertical).margin(fuzz_arg4).constraints([Constraint::Min(fuzz_arg5), Constraint::Length(fuzz_arg6)].as_ref()).split(rect);

case 1

fuzz input : (65535, 18503, 64000, 61439, 64, 255, 18443)

Thread '<unnamed>' panicked at 'attempt to add with overflow', tui-0.19.0/src/layout.rs:422

    pub fn inner(self, margin: &Margin) -> Rect {
        if self.width < 2 * margin.horizontal || self.height < 2 * margin.vertical {
            Rect::default()
        } else {
            Rect {
                x: self.x + margin.horizontal, // overflow!
                y: self.y + margin.vertical,
                width: self.width - 2 * margin.horizontal,
                height: self.height - 2 * margin.vertical,
            }
        }
    }

case 2

fuzz input : (65535, 65535, 65535, 65535, 65535, 65535, 65535)

Thread '<unnamed>' panicked at 'attempt to multiply with overflow', tui-0.19.0/src/layout.rs:418

    pub fn inner(self, margin: &Margin) -> Rect {
        if self.width < 2 * margin.horizontal || self.height < 2 * margin.vertical {  // overflow!
            Rect::default()
        } else {
            Rect {
                x: self.x + margin.horizontal,
                y: self.y + margin.vertical,
                width: self.width - 2 * margin.horizontal,
                height: self.height - 2 * margin.vertical,
            }
        }
    }

case 3

fuzz input : (18504, 65535, 18504, 18504, 1, 18504, 18504)

Thread '<unnamed>' panicked at 'attempt to add with overflow', /home/sslab/rust-fuzz/cache_dir/tui-0.19.0/src/layout.rs:423

    pub fn inner(self, margin: &Margin) -> Rect {
        if self.width < 2 * margin.horizontal || self.height < 2 * margin.vertical {  
            Rect::default()
        } else {
            Rect {
                x: self.x + margin.horizontal,
                y: self.y + margin.vertical,  // overflow!
                width: self.width - 2 * margin.horizontal,
                height: self.height - 2 * margin.vertical,
            }
        }
    }

Expected behavior

Adding assert statement or modifying to use checked operation would resolve this issue.

[joshka]

Please update the info to just the ratatui info (not the tui-rs repo). Tui-rs will likely never receive more updates AFAICT.

You mentioned this affects layout::split, but the code examples are for Rect::inner.

Can you please test these against the main branch rather than the released code, and add a permalink instead of pasting code directly

ratatui/src/layout.rs

Lines 522 to 532 in 5498a88

	pub fn split(&self, area: Rect) -> Rc<[Rect]> {
	LAYOUT_CACHE.with(|c| {
	c.get_or_init(|| {
	RefCell::new(LruCache::new(
	NonZeroUsize::new(Self::DEFAULT_CACHE_SIZE).unwrap(),
	))
	})
	.borrow_mut()
	.get_or_insert((area, self.clone()), || split(area, self))
	.clone()
	})

You might find it quicker to submit a PR to fix some of these fuzz issues rather than type up a full issue.

That arithmetic for inner() should be saturating.

[joshka]

TODO:

• Clarify the failure (include a full backtrace rather than just the last error)
• Replace math in Rect::inner() with saturating math ops

[HeeillWang]

The overflow path starts from Layout::split and it affects on Rect::inner. But yes, it's better to just focus on Rect::inner as it's declared as public API as well.

Sorry for hard-looking sample codes, it's used on fuzzers. I'll simplify the issue and open the pr.

[joshka]

Sorry for hard-looking sample codes, it's used on fuzzers. I'll simplify the issue and open the pr.

I'm not concerned about it. It would be nice to show the code from Ratatui rather than Tui, and if possible with a github permalink (these help show the problem in the context). I wonder how hard it would be to make the fuzzer your're using do that by default (or is the issue text here manually created)? The url format is pretty easy to construct given the filename, lines and git hash. E.g.:
https://github.com/ratatui-org/ratatui/blob/5498a889ae8bd4ccb51b04d3a848dd2f58935906/src/layout.rs#L522-L532

Can you retest this is fixed? What fuzzer are you using for this?

[HeeillWang]

ratatui/src/layout.rs

Lines 228 to 242 in c5ea656

	pub fn inner(self, margin: &Margin) -> Rect {
	let doubled_margin_horizontal = margin.horizontal.saturating_mul(2);
	let doubled_margin_vertical = margin.vertical.saturating_mul(2);
	if self.width < doubled_margin_horizontal || self.height < doubled_margin_vertical {
	Rect::default()
	} else {
	Rect {
	x: self.x.saturating_add(margin.horizontal),
	y: self.y.saturating_add(margin.vertical),
	width: self.width.saturating_sub(doubled_margin_horizontal),
	height: self.height.saturating_sub(doubled_margin_vertical),
	}
	}
	}

This also has been fixed on #523 . We're using rust-fuzz, and fuzz driver code is already given on issue description. Please note that fuzzer launches API starting with Layout::split(), not Rect::inner().

[joshka]

Thanks again for this :)