Panic in debug mode due to overflow

[laurmaedje]

Reproduction:

fn main() {
    let data = include_bytes!("LinLibertine_R.ttf");
    let face = rustybuzz::Face::from_slice(data, 0).unwrap();
    let mut buffer = rustybuzz::UnicodeBuffer::new();
    for _ in 0..250 {
        buffer.push_str("fi");
    }
    rustybuzz::shape(&face, &[], buffer);
}

Panic happens here: https://github.com/RazrFalcon/rustybuzz/blob/e2f88315124d8a83b075979731510741c99feb4c/src/hb/buffer.rs#L532

In release everything works correctly and the output seems to be fine. I'm not sure, but maybe the overflow is intentional in harfbuzz and we should use wrapping_add.

[RazrFalcon]

Overflow is 99% an undefined behavior. We should use wrapping_add only if HB actually expects an overflow. Maybe the bug is present only in rustybuzz to begin with.

@behdad Does overflow expected in uint8_t next_serial () { return ++serial ? serial : ++serial; } or is this a bug?

Also, what this code even does? In RB we have:

        self.serial += 1;
        if self.serial == 0 {
            self.serial += 1;
        }
        self.serial

instead, which is hopefully the same logic, but without C magic.

[LaurenzV]

The "overflow" also happens in harfbuzz, so it's not just in rustybuzz:

So I think we should in fact use a wrapping add.

[CryZe]

Isn't that UB then and should be fixed upstream as well? (C unfortunately doesn't have a builtin wrapping add, at least not via the standard, but there's gnu extensions and co. that have it and worst case you do a check before incrementing, which should optimize away anyway)

[RazrFalcon]

@CryZe Yes, will wait for @behdad response first. After all, unsigned integer overflow in C/C++ is only partially undefined, since we usually know what will happen (wrap around). Therefore it might be an expected behavior. No idea if a compiler can do anything weird here.

[CryZe]

Ah, yeah I forgot that for unsigned integers it's actually fully defined to wrap around. As per C++ 17 § 6.9.1 4:

Unsigned integers shall obey the laws of arithmetic modulo 2^n where n is the number of bits in the value representation of that particular size of integer. 49)

And footnote 49):

This implies that unsigned arithmetic does not overflow because a result that cannot be represented by the resulting unsigned integer type is reduced modulo the number that is one greater than the largest value that can be represented by the resulting unsigned integer type.

So harfbuzz is not doing anything wrong (at least according to the standard, it could still be a logic error) and rustybuzz should be using wrapping_add.

[behdad]

@behdad Does overflow expected in uint8_t next_serial () { return ++serial ? serial : ++serial; } or is this a bug?

Yes, it's expected that it would just wrap around.