Skip to content

Panic self.offset as u64 + len as u64 <= u32::MAX as u64 in src/parser.rs #178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
qarmin opened this issue Oct 21, 2024 · 3 comments
Open

Comments

@qarmin
Copy link

qarmin commented Oct 21, 2024

code

fn check_file(file_path: &str) {
    let Ok(content) = fs::read(file_path) else {
        return;
    };
    let face = match ttf_parser::Face::parse(&content, 0) {
        Ok(f) => f,
        Err(e) => {
            eprintln!("Error: {}.", e);
            return;
        }
    };
    let gid = GlyphId(0);
    let _ = face.glyph_raster_image(gid, 0);
    let _ = face.glyph_raster_image(gid, 96);
    let _ = face.glyph_raster_image(gid, u16::MAX);
    let _ = face.glyph_name(gid);
}

cause this

thread 'main' panicked at /home/runner/.cargo/git/checkouts/ttf-parser-cef4d149453e6ac0/bee14b1/src/parser.rs:770:9:
assertion failed: self.offset as u64 + len as u64 <= u32::MAX as u64
stack backtrace:
   0: rust_begin_unwind
             at /rustc/da935398d582344c5b7689bd6632d8ec01b0c988/library/std/src/panicking.rs:665:5
   1: core::panicking::panic_fmt
             at /rustc/da935398d582344c5b7689bd6632d8ec01b0c988/library/core/src/panicking.rs:74:14
   2: core::panicking::panic
             at /rustc/da935398d582344c5b7689bd6632d8ec01b0c988/library/core/src/panicking.rs:148:5
   3: ttf_parser::parser::Stream::read_bytes
             at /home/runner/.cargo/git/checkouts/ttf-parser-cef4d149453e6ac0/bee14b1/src/parser.rs:770:9
   4: ttf_parser::parser::Stream::read_array32
             at /home/runner/.cargo/git/checkouts/ttf-parser-cef4d149453e6ac0/bee14b1/src/parser.rs:788:9
   5: ttf_parser::ggg::feature_variations::FeatureVariations::parse
             at /home/runner/.cargo/git/checkouts/ttf-parser-cef4d149453e6ac0/bee14b1/src/ggg/feature_variations.rs:23:23
   6: core::ops::function::FnOnce::call_once
             at /home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
   7: core::option::Option<T>::and_then
             at /home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/option.rs:1445:24
   8: ttf_parser::ggg::layout_table::LayoutTable::parse
             at /home/runner/.cargo/git/checkouts/ttf-parser-cef4d149453e6ac0/bee14b1/src/ggg/layout_table.rs:51:22
   9: core::ops::function::FnOnce::call_once
             at /home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
  10: core::option::Option<T>::and_then
             at /home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/option.rs:1445:24
  11: ttf_parser::Face::parse_tables
             at /home/runner/.cargo/git/checkouts/ttf-parser-cef4d149453e6ac0/bee14b1/src/lib.rs:1327:18
  12: ttf_parser::Face::parse
             at /home/runner/.cargo/git/checkouts/ttf-parser-cef4d149453e6ac0/bee14b1/src/lib.rs:1117:21
  13: ttf_parser::check_file
             at ./src/crates/ttf_parser/src/main.rs:30:22
  14: ttf_parser::main
             at ./src/crates/ttf_parser/src/main.rs:23:9
  15: core::ops::function::FnOnce::call_once
             at /home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

##### Automatic Fuzzer note, output status "None", output signal "Some(6)"

compressed.zip

@RazrFalcon
Copy link
Collaborator

You are fuzzing it I see, then this is kinda expected. The panic is caused by integer overflow check. So it works as intended.
Will see how it can be avoided.

@AlexandruIca
Copy link

@RazrFalcon I noticed there's a comment there:

// An integer overflow here on 32bit systems is almost guarantee to be caused
// by an incorrect parsing logic from the caller side.
// Simply using `checked_add` here would silently swallow errors, which is not what we want.

What would be the bad things happening if I used checked_add there for example? Would returning something like Result<Option<&'a [u8]>, StreamParsingError> be needed?

@RazrFalcon
Copy link
Collaborator

Yes. The whole point of this assert is to indicate that the caller (TTF table parsing code) has an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants