6 ms·

Ok, but let's consider the other extreme: a world where no library can ever assume a runtime invariant holds without dynamically checking it first. In this wor

Ok, but let's consider the other extreme: a world where no library can ever assume a runtime invariant holds without dynamically checking it first. In this world, Vec::index() would need to perform not only a bounds check but also a check that the pointer is not NonNull::dangling(). Sure, RawVec is supposed to guarantee that the pointer will not be dangling when cap is >0, but RawVec could have a bug in it. I agree that documenting and returning a PtrWasDanglingError error is not good API design. An InternalError for all such cases seems more reasonable. But at some point we need to be able to assume that certain program invariants hold without checking at all (in a release build).

burntsushi 2y ago
We don't have to live in the extreme though. That's one of the great advantages of Rust. :-) In `regex`, for example, there are certainly some cases where I use `unsafe` to elide those dynamic checks because 1) I couldn't do it in safe code and 2) I got a performance bump from it. But of all the dynamic checks in `regex`, this was an extremely small subset of them. And it makes sense to rely on abstractions like `RawVec` to uphold those guarantees. The point is that you're making that trade-off intentionally and for a specific reason (perf). The idea that I would support dogmatically always checking every runtime invariant everywhere is bonkers. :P In contrast, we have someone here who I responded to that is literally suggesting propagating every possible broken runtime invariant into a public API error value.
- haberman 2y ago
  I am biased towards thinking that low-level libraries should generally avoid panic. That would mean that all invariants are either assumed true or returned as errors to the user. I think this is not an unreasonable design: it's how low-level C libraries are traditionally designed. For example SQLite does what I mentioned and has a single SQLITE_INTERNAL error that is documented as: > The SQLITE_INTERNAL result code indicates an internal malfunction. In a working version of SQLite, an application should never see this result code. If application does encounter this result code, it shows that there is a bug in the database engine. --https://www.sqlite.org/rescode.html#internal https://www.sqlite.org/rescode.html#internal I didn't mean to imply that you are for dogmatic checking of every runtime invariant, but the message that began that thread seems to advocate for that, going so far as to try to detect other buggy code that might have stomped on your memory.
  - burntsushi 2y ago
    SQLite is really a terrible example of anything other than what you can accomplish when you pour enormous resources into a single C library. Its `SQLITE_INTERNAL` error code is atypical in my experience. My recollection is that its tests are an order of magnitude bigger than SQLite itself. It is nowhere near a typical example. I don't think `SQLITE_INTERNAL` is how C libraries are typically designed, and even when they are, that doesn't mean they aren't risking UB in places. PCRE2 has its own `PCRE2_ERROR_INTERNAL` error value too, but it's had its fair share of UB related bugs because C is unsafe-everywhere-by-default. More to the point, the fact that hitting UB-instead-of-abort-or-unwinding is normal C library design is kinda the point: that's almost certainly a good chunk of why you end up with CVEs worse than DoS. How many vulnerabilities would have been significantly limited if C made you opt into explicit bound check elision? > but the message that began that thread seems to advocate for that I agree it is poorly worded. I should have caught that in my initial comment in this thread. The problem here really is the extremes IMO. The extremes are "libraries should never use `unwrap()`" and "libraries should check every runtime invariant at all points and panic when they break." You've gotta use your good judgment to pick and choose when they're appropriate. But I have oodles of `unwrap()` in my Rust libraries. Including in the regex crate's parser. And for sure, some people have hit bugs that manifest as panics. And those could in turn feasibly be DoS problems. But they definitely weren't RCEs, and that's because I used `unwrap()`.
    - erk__ 2y ago
      If zstd give you an error and you don't handle it, the next calls may cause UB, so it kinda does both things. https://github.com/facebook/zstd/blob/b16d193512d3ded82fd584fa822c19ecf67b09a0/lib/zstd.h#L942 https://github.com/facebook/zstd/blob/b16d193512d3ded82fd584...
    - haberman 2y ago
      > Its `SQLITE_INTERNAL` error code is atypical in my experience. In my experience it's reasonably common. Here are some other examples in what I would consider quintessential, high-quality C libraries: - zlib has Z_STREAM_ERROR, which is documented in several places as being returned "if the stream structure was inconsistent" - libavcodec has AVERROR_BUG, documented as "Internal bug, also see AVERROR_BUG2". - LMDB has MDB_PANIC, documented as "Update of meta page failed or environment had fatal error". > And for sure, some people have hit bugs that manifest as panics. And those could in turn feasibly be DoS problems. But they definitely weren't RCEs, and that's because I used `unwrap()`. I feel this is conflating two things: (1) whether or not an invariant should get a dynamic check, and (2) when a dynamic check is present, how the failure should be reported. Rust brings safety by forcing (safe) code to use dynamic checks when a safety property cannot be statically guaranteed, which addresses (1). But there's still a degree of freedom for whether failures are reported as panics or as recoverable errors to the caller. I wrote down some of my thinking in this recent blog entry, which actually quotes your excellent summary of when panics are appropriate: https://blog.reverberate.org/2025/02/03/no-panic-rust.html https://blog.reverberate.org/2025/02/03/no-panic-rust.html (ps: I'm a daily rg user and fan of your work!)

burntsushi 2y ago

We don't have to live in the extreme though. That's one of the great advantages of Rust. :-) In `regex`, for example, there are certainly some cases where I use `unsafe` to elide those dynamic checks because 1) I couldn't do it in safe code and 2) I got a performance bump from it. But of all the dynamic checks in `regex`, this was an extremely small subset of them. And it makes sense to rely on abstractions like `RawVec` to uphold those guarantees. The point is that you're making that trade-off intentionally and for a specific reason (perf). The idea that I would support dogmatically always checking every runtime invariant everywhere is bonkers. :P In contrast, we have someone here who I responded to that is literally suggesting propagating every possible broken runtime invariant into a public API error value.

haberman 2y ago
I am biased towards thinking that low-level libraries should generally avoid panic. That would mean that all invariants are either assumed true or returned as errors to the user. I think this is not an unreasonable design: it's how low-level C libraries are traditionally designed. For example SQLite does what I mentioned and has a single SQLITE_INTERNAL error that is documented as: > The SQLITE_INTERNAL result code indicates an internal malfunction. In a working version of SQLite, an application should never see this result code. If application does encounter this result code, it shows that there is a bug in the database engine. --https://www.sqlite.org/rescode.html#internal https://www.sqlite.org/rescode.html#internal I didn't mean to imply that you are for dogmatic checking of every runtime invariant, but the message that began that thread seems to advocate for that, going so far as to try to detect other buggy code that might have stomped on your memory.
- burntsushi 2y ago
  SQLite is really a terrible example of anything other than what you can accomplish when you pour enormous resources into a single C library. Its `SQLITE_INTERNAL` error code is atypical in my experience. My recollection is that its tests are an order of magnitude bigger than SQLite itself. It is nowhere near a typical example. I don't think `SQLITE_INTERNAL` is how C libraries are typically designed, and even when they are, that doesn't mean they aren't risking UB in places. PCRE2 has its own `PCRE2_ERROR_INTERNAL` error value too, but it's had its fair share of UB related bugs because C is unsafe-everywhere-by-default. More to the point, the fact that hitting UB-instead-of-abort-or-unwinding is normal C library design is kinda the point: that's almost certainly a good chunk of why you end up with CVEs worse than DoS. How many vulnerabilities would have been significantly limited if C made you opt into explicit bound check elision? > but the message that began that thread seems to advocate for that I agree it is poorly worded. I should have caught that in my initial comment in this thread. The problem here really is the extremes IMO. The extremes are "libraries should never use `unwrap()`" and "libraries should check every runtime invariant at all points and panic when they break." You've gotta use your good judgment to pick and choose when they're appropriate. But I have oodles of `unwrap()` in my Rust libraries. Including in the regex crate's parser. And for sure, some people have hit bugs that manifest as panics. And those could in turn feasibly be DoS problems. But they definitely weren't RCEs, and that's because I used `unwrap()`.
  - erk__ 2y ago
    If zstd give you an error and you don't handle it, the next calls may cause UB, so it kinda does both things. https://github.com/facebook/zstd/blob/b16d193512d3ded82fd584fa822c19ecf67b09a0/lib/zstd.h#L942 https://github.com/facebook/zstd/blob/b16d193512d3ded82fd584...
  - haberman 2y ago
    > Its `SQLITE_INTERNAL` error code is atypical in my experience. In my experience it's reasonably common. Here are some other examples in what I would consider quintessential, high-quality C libraries: - zlib has Z_STREAM_ERROR, which is documented in several places as being returned "if the stream structure was inconsistent" - libavcodec has AVERROR_BUG, documented as "Internal bug, also see AVERROR_BUG2". - LMDB has MDB_PANIC, documented as "Update of meta page failed or environment had fatal error". > And for sure, some people have hit bugs that manifest as panics. And those could in turn feasibly be DoS problems. But they definitely weren't RCEs, and that's because I used `unwrap()`. I feel this is conflating two things: (1) whether or not an invariant should get a dynamic check, and (2) when a dynamic check is present, how the failure should be reported. Rust brings safety by forcing (safe) code to use dynamic checks when a safety property cannot be statically guaranteed, which addresses (1). But there's still a degree of freedom for whether failures are reported as panics or as recoverable errors to the caller. I wrote down some of my thinking in this recent blog entry, which actually quotes your excellent summary of when panics are appropriate: https://blog.reverberate.org/2025/02/03/no-panic-rust.html https://blog.reverberate.org/2025/02/03/no-panic-rust.html (ps: I'm a daily rg user and fan of your work!)