search

connorskees / grass

A Sass compiler written purely in Rust
https://docs.rs/grass/
MIT License
499 stars 38 forks source link

Type conversion between i64 and usize #101

Closed qaqland closed 2 months ago

qaqland commented 2 months ago

https://github.com/connorskees/grass/blob/e0bb9e2eabfc3a58e42b03089cd7b22c68d09d0b/crates/compiler/src/builtin/functions/string.rs#L149

here the limit_int is i64 but the following use it as usize

https://github.com/connorskees/grass/blob/e0bb9e2eabfc3a58e42b03089cd7b22c68d09d0b/crates/compiler/src/builtin/functions/string.rs#L158

On 32-bit platforms, there is an overflow risk

connorskees commented 2 months ago

Hi, thanks for reporting this. Interesting find.

Although in theory it is possible for the integer to overflow, I don't think this is a cause for concern. Are you running into this in practice, or is this just something you found by looking/scanning the code? I would argue splitting a string at index 4 billion is malformed input.

That said, I would accept a PR to change 1 + limit_int into limit_int.saturating_add(1).

qaqland commented 2 months ago

https://github.com/connorskees/grass/blob/e0bb9e2eabfc3a58e42b03089cd7b22c68d09d0b/crates/lib/tests/strings.rs#L338-L343

this test in pipeline failed

https://gitlab.alpinelinux.org/qaqland/aports/-/pipelines/256962

connorskees commented 2 months ago

Oh, that makes sense. I can release a fix for this to crates.io

qaqland commented 2 months ago

btw, could you please update the version of libc in Cargo.lock, it supports loongarch after 0.2.155