3 ms·

Now that I've slept, I decided to take a look at LMDB. It uses MDB_PANIC in exactly two places: https://github.com/LMDB/lmdb/blob/f20e41de09d97e4461946b7e26ec8

Now that I've slept, I decided to take a look at LMDB. It uses MDB_PANIC in exactly two places: https://github.com/LMDB/lmdb/blob/f20e41de09d97e4461946b7e26ec831d0c24fac7/libraries/liblmdb/mdb.c#L3182 https://github.com/LMDB/lmdb/blob/f20e41de09d97e4461946b7e26... https://github.com/LMDB/lmdb/blob/f20e41de09d97e4461946b7e26ec831d0c24fac7/libraries/liblmdb/mdb.c#L11420 https://github.com/LMDB/lmdb/blob/f20e41de09d97e4461946b7e26... I would say this overall does not even come close to qualifying as an example of a library that "returns errors for invariant violations instead of committing UB." You don't have to look far to see something that would normally be a panicking branch in Rust be a UB branch in C: https://github.com/LMDB/lmdb/blob/f20e41de09d97e4461946b7e26ec831d0c24fac7/libraries/liblmdb/mdb.c#L1771-L1774 https://github.com/LMDB/lmdb/blob/f20e41de09d97e4461946b7e26... if (err >= MDB_KEYEXIST && err <= MDB_LAST_ERRCODE) { i = err - MDB_KEYEXIST; return mdb_errstr[i]; } That `mdb_errstr[i]` will have UB if `i` is out of bounds. And `i` could be out of bounds if this code gets out of sync with the defined error constants and `mdb_errstr`. Moreover, it seems quite unlikely that this particular part of the code benefits perf-wise from omitting bounds checks. In other words, if this were Rust code and someone used `unsafe` to opt out of bounds checks here (assuming they weren't already elided automatically), that would be a gross error in judgment IMO. The kind of examples I'm asking for would be C libraries that catch these sorts of runtime invariants and propagate them up as errors. Instead, at least for LMDB, MDB_PANIC isn't really used for this purpose. Now looking at zlib, from what I can tell, Z_STREAM_ERROR is used to validate input arguments. It's not actually being used to detect runtime invariants. zlib is just like most any other C library as far as I can tell. There are UB branches everywhere. I'm sure some of those are important for perf, but I've spent 10 years working on optimizing low level libraries in Rust, and I can say for certain that the vast majority of them are not. libavcodec is more of the same. There are a ton of runtime invariants everywhere that are just UB if they are broken. Again, this is not an example of a library eagerly checking for invariant violations and percolating up errors. From what I can see, AVERROR_BUG is used at various boundaries to detect some kinds of inconsistencies in the data. IMO, your examples are a total misrepresentation of how C libraries typically work. From my review, my prior was totally confirmed: C libraries will happily do UB when runtime invariants are broken, where as Rust code tends to panic. Rust code will opt into the "UB when runtime invariants are broken," but it is far far more limited. And this further demonstrates why "unsafe by default" is so bad.

haberman 2y ago
I think this is moving the goalposts. My claim was not "these C libraries perfectly avoid UB by dynamically checking every invariant that could lead to UB if broken." Clearly they do not, as you have demonstrated. (Neither does unsafe Rust). My claim was that in cases where a (low-level, high quality) C library does check an invariant in a release build, it will generally report failure of that invariant as an explicit error code rather than by crashing the process. To falsify that, you would need to find places where these libraries call abort() or exit() in response to an internal inconsistency, in a release build. I think you are unlikely to find examples of that in these libraries. (After a bit of searching, I see that libavcodec has a few abort()s, but uses AVERROR_BUG an order of magnitude more often). I agree with you that Rust's "safe by default" is important. I am advocating that Rust can be a powerful tool to provide C-like behavior (no crash on inconsistency) with greater safety (checking all relevant inconsistencies by default). In cases where C-like behavior is desired, that's a really appealing proposition. Upthread it seemed like you were objecting to the idea of ever reporting internal inconsistencies as recoverable errors. You argued that creating and documenting error codes for this is not common or practical: > 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." That is exactly what SQLITE_INTERNAL and AVERROR_BUG are.
- burntsushi 2y ago
  > My claim was that in cases where a (low-level, high quality) C library does check an invariant in a release build, it will generally report failure of that invariant as an explicit error code rather than by crashing the process. That just seems very uninteresting though? And it kinda misses the whole point of where this conversation started. It's true that Rust code is going to check more things because of `unwrap()`, but that's a good thing! Because the alternative is clearly what C libraries practice: they'll just have UB. So you give up the possibility of an RCE for the possibility of a DoS. Sounds like a good trade to me. >> 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." > > That is exactly what SQLITE_INTERNAL and AVERROR_BUG are. I meant that it should reflect the philosophy of handling broken runtime invariants generally in the library. Just because there's one error code for some restricted subset of cases doesn't mean that's how they deal with broken runtime invariants. In all of your examples so far, the vast majority of broken runtime variants from what I can see lead to UB, not error codes. This is what I meant because this is what makes Rust and its panicking materially different from C. And it's relevant especially in contexts where people say, "well just return an error instead of panicking." But C libraries generally don't do that either! They don't even bother checking most runtime invariants anyway, even when it doesn't matter for perf. This is a big knot to untangle and I'm sure my wording could have been more precise. This is why I wanted to focus on examples, because we can look at real world things. And from my perspective, the examples you've given do not embody the original advice that I was replying to: > That behavior is up to the user. The library should only report the error. Instead, while there is limited support for "this error is a bug," the C libraries you've linked overwhelming prefer UB. That's the relevant point of comparison. I'm not interested in trying to find C libraries that abort. I'm interested in a holistic comparison of actual practice and using that to contextualize the blanket suggestions given in this thread.

haberman 2y ago

I think this is moving the goalposts. My claim was not "these C libraries perfectly avoid UB by dynamically checking every invariant that could lead to UB if broken." Clearly they do not, as you have demonstrated. (Neither does unsafe Rust). My claim was that in cases where a (low-level, high quality) C library does check an invariant in a release build, it will generally report failure of that invariant as an explicit error code rather than by crashing the process. To falsify that, you would need to find places where these libraries call abort() or exit() in response to an internal inconsistency, in a release build. I think you are unlikely to find examples of that in these libraries. (After a bit of searching, I see that libavcodec has a few abort()s, but uses AVERROR_BUG an order of magnitude more often). I agree with you that Rust's "safe by default" is important. I am advocating that Rust can be a powerful tool to provide C-like behavior (no crash on inconsistency) with greater safety (checking all relevant inconsistencies by default). In cases where C-like behavior is desired, that's a really appealing proposition. Upthread it seemed like you were objecting to the idea of ever reporting internal inconsistencies as recoverable errors. You argued that creating and documenting error codes for this is not common or practical: > 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." That is exactly what SQLITE_INTERNAL and AVERROR_BUG are.

burntsushi 2y ago
> My claim was that in cases where a (low-level, high quality) C library does check an invariant in a release build, it will generally report failure of that invariant as an explicit error code rather than by crashing the process. That just seems very uninteresting though? And it kinda misses the whole point of where this conversation started. It's true that Rust code is going to check more things because of `unwrap()`, but that's a good thing! Because the alternative is clearly what C libraries practice: they'll just have UB. So you give up the possibility of an RCE for the possibility of a DoS. Sounds like a good trade to me. >> 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." > > That is exactly what SQLITE_INTERNAL and AVERROR_BUG are. I meant that it should reflect the philosophy of handling broken runtime invariants generally in the library. Just because there's one error code for some restricted subset of cases doesn't mean that's how they deal with broken runtime invariants. In all of your examples so far, the vast majority of broken runtime variants from what I can see lead to UB, not error codes. This is what I meant because this is what makes Rust and its panicking materially different from C. And it's relevant especially in contexts where people say, "well just return an error instead of panicking." But C libraries generally don't do that either! They don't even bother checking most runtime invariants anyway, even when it doesn't matter for perf. This is a big knot to untangle and I'm sure my wording could have been more precise. This is why I wanted to focus on examples, because we can look at real world things. And from my perspective, the examples you've given do not embody the original advice that I was replying to: > That behavior is up to the user. The library should only report the error. Instead, while there is limited support for "this error is a bug," the C libraries you've linked overwhelming prefer UB. That's the relevant point of comparison. I'm not interested in trying to find C libraries that abort. I'm interested in a holistic comparison of actual practice and using that to contextualize the blanket suggestions given in this thread.