[fuzz-test] Arithmetic overflow occurs while using API Buffer::empty()

[HeeillWang]

Description

I executed fuzzing to test tui-0.19.0 APIs. This reproduced on ratatui as well.

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

    pub fn area(self) -> u16 {
        self.width * self.height //overflow!
    }

To Reproduce

let mut fuzz_arg8:u16 = 65280;
let mut fuzz_arg9:u16 = 65535;
let mut fuzz_arg10:u16 = 65535;
let mut fuzz_arg11:u16 = 65535;
Buffer::empty(Rect { x:fuzz_arg11, y:fuzz_arg9, width:fuzz_arg10, height:fuzz_arg8 });

Expected behavior

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

[a-kenji]

Adding assert statement

That might be a reasonable addition.
At least it could make sense to add documentation to the area function.

modifying to use checked operation

While I don't think any terminal can get this big and actually function, maybe also providing a checked version of this function does make some sense: area_checked.

[joshka]

Area should possibly just be u32 or perhaps usize instead to allow for larger dimensions. Note: I haven't checked the history of this decision to understand if there is a good rationale for this. It seems intentional that we avoid allocating more than u16::MAX for any Rect as noted by the ::new() function.

[HeeillWang]

Adding assert still meaningful, as arith overflow does not panic by rustc on release build.

Plus, in this particular case, modifying field of Rect to private or pub(crate) can resolve this issue as well. i.e. force external users to use Rect::new() instead. Though I'm not very sure if there is another overflow path to area() or not...

[joshka]

arith overflow does not panic by rustc on release build.

TIL (thanks).

Adding assert still meaningful

By definition, u16 * u16 can't overflow u32 / usize. Regardless, the sort of use case where area() is called from is more likely to be for 0 checks rather than > u16::MAX checks. I think a better stopgap solution would be to use saturating ops rather than panicking.

modifying field of Rect to private or pub(crate) can resolve this issue as well. i.e. force external users to use Rect::new() instead

There's a lot of reasonable uses in apps to want to do e.g.:

let sub_area = Rect { x: area.x + 1, width: area.x - 1, ..area };

[HeeillWang]

Using saturating ops seems best fit for this case. Thanks.

I'll come up with pr for this issue and #520

[joshka]

Can you please re-run the fuzzer with the updated code to check that this is fixed and there aren't other issues in the same area?

[HeeillWang]

This has been fixed on #523 , no other issues has been found.