console-rs / indicatif

A command line progress reporting library for Rust
MIT License
4.22k stars 238 forks source link

Attempt to subtract with overflow when output exceeds terminal height #582

Closed smoelius closed 9 months ago

smoelius commented 10 months ago

I am observing attempt to subtract with overflow on this line: https://github.com/console-rs/indicatif/blob/40b40d29b6d06ae18c40b829f77e9c43bcebd7af/src/draw_target.rs#L534

self.lines.len() >= self.orphan_lines_count holds prior to entering this loop: https://github.com/console-rs/indicatif/blob/40b40d29b6d06ae18c40b829f77e9c43bcebd7af/src/draw_target.rs#L501

And real_len >= idx holds at the start of each iteration of that loop.

Those two facts causes me to squint at this if block: https://github.com/console-rs/indicatif/blob/40b40d29b6d06ae18c40b829f77e9c43bcebd7af/src/draw_target.rs#L517-L519

In particular, commenting out that if block causes my panics to go away.

I don't completely understand the purpose of that if block, but maybe it can go away?

(indicatif is a great tool, BTW.)

smoelius commented 10 months ago

Here are steps to reproduce the bug.

  1. Create a debug build of Necessist, e.g.:

    git clone https://github.com/trailofbits/necessist
    cd necessist
    cargo build
  2. Create a project with a test like this:

    #[test]
    fn many_lines() {
    let mut s = String::new();
    for _ in 0..100 {
        s.push('\n');
    }
    None::<()>.expect(&s);
    }
  3. Within the project containing the above test, run Necessist:

    path-to-clone/target/debug/necessist

You should see something like this:

thread 'main' panicked at 'attempt to subtract with overflow', .../.cargo/registry/src/index.crates.io-6f17d22bba15001f/indicatif-0.17.6/src/draw_target.rs:534:28
djc commented 10 months ago

@smoelius thanks for the detailed issue and reproduction case. Unfortunately I don't have much time to dig into this in detail at the moment. The code you're referencing here was introduced in #563, maybe read up on that to figure out if/why the branch was necessary? Probably the easiest fix is to just slap a saturating_sub() on it, and I'm happy to merge that PR, but if you want to look into it a bit more to figure out the proper invariants (and add some comments to document them for future readers) that would of course be much appreciated!

smoelius commented 10 months ago

Thank you very much for providing the background. I will try to look into this more and propose a fix.