An unsafety bug in rust's stdlib
Rust 1.29.1 was released Sept 25, 2018. The release announcement mentions a security vulnerability. It says something about buffer overflow, which seemed odd to me. There's a link to an email thread describing it. It's a typical description of a buffer overflow bug.
This vulnerability is an instance of CWE-680: Integer Overflow to Buffer Overflow[1].
The
str::repeatfunction in the standard library allows repeating a string a fixed number of times, returning an owned version of the final string. The capacity of the final string is calculated by multiplying the length of the string being repeated by the number of copies. This calculation can overflow, and this case was not properly checked for.The rest of the implementation of
str::repeatcontains unsafe code that relies on a preallocated vector having the capacity calculated earlier. On integer overflow the capacity will be less than required, and which then writes outside of the allocated buffer, leading to buffer overflow.
Rust is one of several modern languages lauded as "memory safe" and so this kind of error should be fairly rare. I was curious about the code, and the email thread continues:
While the
str::repeatfunction has been in Rust since 1.16.0, this vulnerability was introduced into the standard library in pull request #48657 [2]. The pull request was merged on March 6, 2018 and was first part of the 1.26.0 stable released on May 10, 2018.
Let's open up that PR https://github.com/rust-lang/rust/pull/48657/. It looks like this change makes a reasonable optimization. I have no context for what prompted the community to desire this optimization. It's a good speedup. All seems reasonable still.
Flip over to the files tab, to see the diff and it starts to seem a little strange. This change takes a concise and clear functional-style implementation and updates it to a long, complex, and apparently buggy imperative-style.
This seems like a fairly niche use-case function, and yet it became the vector for a rather serious bug. It makes me wonder why the function exists in the standard library at all, or why it was desirable to update it. I'm biased toward functional programming, and this seems to "confirm my bias". Perhaps there's more to the story? Sorry for stepping in without context.
- What does the community think of the story of this bug? Is there more to it?
- When are optimizations which reduce readability justified?
- What values should a programming language's standard library prioritize?