9 ms·

That behavior is up to the user. The library should only report the error.

burntsushi 2y ago
This is terrible advice, and people suggesting it should put their money where their mouth it and show real world examples. Which libraries in widespread use know how to detect all of their possible bugs due to invariant violations and report them as explicit error values? I'd love to see the API docs for them. "This error value is impossible and this library will never return it. If it does, then there is a bug in the library. Since there are no known bugs related to this invariant violation, this cannot happen." Nevermind the fact that you're now potentially introducing an error channel into operations that should never actually error. That potentially has performance implications. Nevermind the fact that now your implementation details (internal runtime invariants are implementation details) have now leaked out into your API. Need a new internal runtime invariant? Now you need to add that fact to the API. Need to remove an invariant? Ah well, you still need to leave that possible error value in your API to avoid breaking your users.
- haberman 2y ago
  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()`.
- theamk 2y ago
  That's how exceptions work, and they come pretty handy in a lot of circumstances. In such languages, any operation might throw RuntimeException (or equivalent) and the caller must be ready to handle that - or not, in which case it behaves exactly like non-exception-supported language. I know that a lot of people hate that idea, but I strongly disagree. In any large programs, there are thousands of possible errors, and only a small part of them we actually want to handle in a special way. The rest? They go to "other" category. Being able to handle "other" errors, what Rust calls "panic", significantly improves user experience: For CLI, print explanation that this is an unexpected failure, mention where the logs were saved, mention where to get support (forum/issue/etc...), and exit. For cron-like scheduled service, notify oncall of the crash, re-schedule the job with intelligent timeout, then exit. For web, upload details to observability platform, return 500 to user, then when possible terminate the worker. and so on... In practical world, unexpected errors are a thing, and good language should support them to make programmers' lives easier. One unfortunate downside of this ability is that some programmers abuse it, and ignore all the unknown errors instead of handling them properly - this makes a terrible user UX and introduces many bugs. Also, for my "web services" example, if the worker is not terminated, there is a chance the internal data structures will get corrupted, and further requests, even the ones which used to pass, will now fail. There are ways to mitigate this - ignore some exception groups but unconditionally fail on others; or use try/finally blocks and immutable data to reduce the chance of corruption even in case of unexpected exception. But this code is hard to argue about and hard to test for. Still, if a feature is not a good idea in some specific circumstances, it's not a reason to remove it altogether.
  - burntsushi 2y ago
    I'm unsure of your point here. And I'm not getting dragged into a debate about exceptions. :-)
    - edflsafoiewq 2y ago
      I think the point is very clear. Languages with exceptions report invariant violations like IndexOutOfBoundsError or AssertionFailed via the same error reporting mechanism as normal, unavoidable errors, namely by throwing exceptions.
- hansvm 2y ago
  > Which libraries in widespread use know how to detect all of their possible bugs due to invariant violations and report them as explicit error values? We're talking about the cases that are already being caught somehow (bounds checks, unwraps, ...). It isn't necessary to detect all possible invariant violations to do something else instead of panic, and it suffices to have the language represent those failures without aborting the program.
  - burntsushi 2y ago
    Show me a widely used C library that does even remotely the same thing. I promise you most places where Rust would use unwrap are just straight UB in C. I note that you provided no real world examples despite my request for them. Where's your code that is following this advice of yours?
    - hansvm 2y ago
      I'm not advocating for C or against Rust though. I'm saying that GP's request to report errors instead of crashing is a perfectly fine opinion, and using Rust as an example of a language which already traps most instances of C UB, there aren't any fundamental reasons why Rust (or a fork or a similar language) couldn't use a different mechanism to signal failure states. Your request for code is irrelevant to my point.
      - burntsushi 2y ago
        You advocate a particular coding style and I ask for real world examples demonstrating your advocacy in the real world. That's absolutely relevant! In contrast, the style I advocate has dozens of examples at your fingertips running in production right now. Including the Rust standard library itself. The Rust standard library happily uses `unwrap()` all over the place and specifically does not propagate errors that are purely the result of bugs coming from broken internal runtime invariants.
- anon-3988 2y ago
  > Which libraries in widespread use know how to detect all of their possible bugs due to invariant violations and report them as explicit error values? Its only "hard" because languages make it hard and implicit what the std library itself is doing. At worst, every single "assert" should be an exception. Not ALL possible invariant but wrong array index is a lot of them? If Rust's Vec::operator[] or allocation (new, default) all returns an Result<T, E>. Then its a matter of adding another invariant to the libraries' error enum. I don't have anything public really to show this but at my work, our library written in C++ have like gazillions of checks using macros, every single invariant is checked and they all crash at debug and throw exceptions at release. The idea is to eventually trim them down after its been stabilized? Just kidding! The reality is that it actually caught bugs introduced by changes multiple times so it will probably be there forever.
  - burntsushi 2y ago
    Neither C nor Rust have exceptions. Rust has unwinding, which are kind of like exceptions. And indeed `assert!` will do unwinding (although applications can be built in a way where it will be an abort) that can be caught. > Its only "hard" because languages make it hard and implicit what the std library itself is doing. This is an extraordinary claim that requires extraordinary evidence. I absolutely don't buy this for one second. > I don't have anything public really to show this but at my work, our library written in C++ have like gazillions of checks using macros, every single invariant is checked and they all crash at debug and throw exceptions at release. This isn't good enough to demonstrate that your advice is broadly applicable. Nowhere near good enough. Other people in this thread provided examples of what they thought were the same thing, but it actually turned out that most broken internal runtime invariants would just lead to UB, despite having error codes like "this only gets returned if there is a bug." So I can't tell if you're making the same mischaracterization. Moreover, you don't state the domain you're in. I could absolutely believe that there are some domains where it is acceptable to invest huge resources into eliminating all possible aborts, even for broken internal runtime invariants. But I'd expect them also to eliminate all possible instances of UB as well, because, well, UB can result in aborts! (And often does, via a segmentation fault.) At this point, you're in "prove your code is correct" territory. Sometimes that's warranted, but your comment was made without any of this nuance at all.

This is terrible advice, and people suggesting it should put their money where their mouth it and show real world examples. Which libraries in widespread use know how to detect all of their possible bugs due to invariant violations and report them as explicit error values? I'd love to see the API docs for them. "This error value is impossible and this library will never return it. If it does, then there is a bug in the library. Since there are no known bugs related to this invariant violation, this cannot happen." Nevermind the fact that you're now potentially introducing an error channel into operations that should never actually error. That potentially has performance implications. Nevermind the fact that now your implementation details (internal runtime invariants are implementation details) have now leaked out into your API. Need a new internal runtime invariant? Now you need to add that fact to the API. Need to remove an invariant? Ah well, you still need to leave that possible error value in your API to avoid breaking your users.

haberman 2y ago
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()`.
theamk 2y ago
That's how exceptions work, and they come pretty handy in a lot of circumstances. In such languages, any operation might throw RuntimeException (or equivalent) and the caller must be ready to handle that - or not, in which case it behaves exactly like non-exception-supported language. I know that a lot of people hate that idea, but I strongly disagree. In any large programs, there are thousands of possible errors, and only a small part of them we actually want to handle in a special way. The rest? They go to "other" category. Being able to handle "other" errors, what Rust calls "panic", significantly improves user experience: For CLI, print explanation that this is an unexpected failure, mention where the logs were saved, mention where to get support (forum/issue/etc...), and exit. For cron-like scheduled service, notify oncall of the crash, re-schedule the job with intelligent timeout, then exit. For web, upload details to observability platform, return 500 to user, then when possible terminate the worker. and so on... In practical world, unexpected errors are a thing, and good language should support them to make programmers' lives easier. One unfortunate downside of this ability is that some programmers abuse it, and ignore all the unknown errors instead of handling them properly - this makes a terrible user UX and introduces many bugs. Also, for my "web services" example, if the worker is not terminated, there is a chance the internal data structures will get corrupted, and further requests, even the ones which used to pass, will now fail. There are ways to mitigate this - ignore some exception groups but unconditionally fail on others; or use try/finally blocks and immutable data to reduce the chance of corruption even in case of unexpected exception. But this code is hard to argue about and hard to test for. Still, if a feature is not a good idea in some specific circumstances, it's not a reason to remove it altogether.
- burntsushi 2y ago
  I'm unsure of your point here. And I'm not getting dragged into a debate about exceptions. :-)
  - edflsafoiewq 2y ago
    I think the point is very clear. Languages with exceptions report invariant violations like IndexOutOfBoundsError or AssertionFailed via the same error reporting mechanism as normal, unavoidable errors, namely by throwing exceptions.
hansvm 2y ago
> Which libraries in widespread use know how to detect all of their possible bugs due to invariant violations and report them as explicit error values? We're talking about the cases that are already being caught somehow (bounds checks, unwraps, ...). It isn't necessary to detect all possible invariant violations to do something else instead of panic, and it suffices to have the language represent those failures without aborting the program.
- burntsushi 2y ago
  Show me a widely used C library that does even remotely the same thing. I promise you most places where Rust would use unwrap are just straight UB in C. I note that you provided no real world examples despite my request for them. Where's your code that is following this advice of yours?
  - hansvm 2y ago
    I'm not advocating for C or against Rust though. I'm saying that GP's request to report errors instead of crashing is a perfectly fine opinion, and using Rust as an example of a language which already traps most instances of C UB, there aren't any fundamental reasons why Rust (or a fork or a similar language) couldn't use a different mechanism to signal failure states. Your request for code is irrelevant to my point.
    - burntsushi 2y ago
      You advocate a particular coding style and I ask for real world examples demonstrating your advocacy in the real world. That's absolutely relevant! In contrast, the style I advocate has dozens of examples at your fingertips running in production right now. Including the Rust standard library itself. The Rust standard library happily uses `unwrap()` all over the place and specifically does not propagate errors that are purely the result of bugs coming from broken internal runtime invariants.
anon-3988 2y ago
> Which libraries in widespread use know how to detect all of their possible bugs due to invariant violations and report them as explicit error values? Its only "hard" because languages make it hard and implicit what the std library itself is doing. At worst, every single "assert" should be an exception. Not ALL possible invariant but wrong array index is a lot of them? If Rust's Vec::operator[] or allocation (new, default) all returns an Result<T, E>. Then its a matter of adding another invariant to the libraries' error enum. I don't have anything public really to show this but at my work, our library written in C++ have like gazillions of checks using macros, every single invariant is checked and they all crash at debug and throw exceptions at release. The idea is to eventually trim them down after its been stabilized? Just kidding! The reality is that it actually caught bugs introduced by changes multiple times so it will probably be there forever.
- burntsushi 2y ago
  Neither C nor Rust have exceptions. Rust has unwinding, which are kind of like exceptions. And indeed `assert!` will do unwinding (although applications can be built in a way where it will be an abort) that can be caught. > Its only "hard" because languages make it hard and implicit what the std library itself is doing. This is an extraordinary claim that requires extraordinary evidence. I absolutely don't buy this for one second. > I don't have anything public really to show this but at my work, our library written in C++ have like gazillions of checks using macros, every single invariant is checked and they all crash at debug and throw exceptions at release. This isn't good enough to demonstrate that your advice is broadly applicable. Nowhere near good enough. Other people in this thread provided examples of what they thought were the same thing, but it actually turned out that most broken internal runtime invariants would just lead to UB, despite having error codes like "this only gets returned if there is a bug." So I can't tell if you're making the same mischaracterization. Moreover, you don't state the domain you're in. I could absolutely believe that there are some domains where it is acceptable to invest huge resources into eliminating all possible aborts, even for broken internal runtime invariants. But I'd expect them also to eliminate all possible instances of UB as well, because, well, UB can result in aborts! (And often does, via a segmentation fault.) At this point, you're in "prove your code is correct" territory. Sometimes that's warranted, but your comment was made without any of this nuance at all.