Hacker News new | ask | show | jobs
by steveklabnik 1569 days ago
This seems to be the patch: https://github.com/github/cmark-gfm/commit/cf7577d2f74289cb8...

Integer overflow can happen in Rust, but it's well-defined, not undefined. This helps.

Bounds checking is part of indexing, and so even if an index overflows, the check should happen, and panic.

"impossible" is a strong word, but it would be significantly less likely in Rust. If you did the same thing as you did in C, with unsafe, then it could happen. But there's not a lot of reason to 99.9999% of the time, as it's the more difficult and less ergonomic option.

3 comments

Is this unsigned integer overflow? Isn’t that well defined in C++ as well?

Edit: I didn’t research where the corruption comes from in this bug.

Edit again: it looks like the source file is actually C and not C++.

Yep, well-defined in C++, but the resulting out-of-bounds accesses and all that are not well-defined.
Thanks. I should look at the code. I thought unsigned int overflow would wrap to zero, which would still be in bounds for a nontrivial array. Maybe they’re freeing the item at that index the first time through the array or something.
Well defined and it also panics in debug mode. Unit tests tend not to catch these sorts of bugs tbh, but still, nice to have :)
the actual commit fix has some comments that may be useful for understanding:

https://github.com/github/cmark-gfm/commit/ac80f7b56522ffa15...

I’m in my phone now but they cut two different patches to two different releases, I suspect I linked to one and you the other. Harder to double check that when I’m not at a computer, though that does have far better comments and I should have linked to it, thank you. I basically picked one at random.
no worries, it's the same patch to two different releases; you linked to the merge commit & I linked to the fix commit.

source: i cut the releases ;)

Ah ha! I should have realized. Thank you for your hard work, this kind of thing is never easy.