BufWriter is not panic-safe

[dgrunwald]

There is a panic safety issue in BufWriter: after a inner.write() call panics, the Drop impl of BufWriter calls inner.write() again, which means the buffer contents are potentially written twice. This may cause an application to overwrite parts of a file that it did not mean to overwrite (in a DB engine written in Rust, this could cause unrecoverable data corruption!).

Demonstration: https://play.rust-lang.org/?gist=9991550d3efb38c93df4&version=stable
The expected output of the demo program is File contents: aBBccc, the actual output is: File contents: aBBBBc

More generally, we need a story for panic safety in Rust.
My takeaway from the related discussions (e.g. RFC 1236, #27719, the RecoverSafe trait) was that only unsafe code and Drop impls should have to worry about panic safety. The demo app contains none of these, so I'd consider this a bug in impl Drop for BufWriter. (otherwise all Write implementations would need to provide the strong exception safety guarantee?)

Solution: BufWriter could use temporarily mark the buffer as empty during the inner.write calls; so that the Drop impl doesn't do anything after a panic.

However, this doesn't help if the panic occurs during a bufWriter.get_mut().write() call...

[Aatch]

Note that Rust is normally referring to "memory safety" when talking about safety, so the issue here isn't in conflict with the related discussions.

It's definitely a bug in BufWriter, but I want to point out how specific it is here: the only reason you have odd behaviour is because you have a Writer implementation that will panic after successfully writing the data it is given. I'm all for making BufWriter more robust, but it's not just "if the inner.write call panics", it's "if the inner.write call panics after successfully writing data". Panicking before a successful write (or if the write fails), would not have the same issue.

[sfackler]

(in a DB engine written in Rust, this could cause unrecoverable data corruption!)

If your Write impl is panicking in the middle of a write, it seems to me like you're in data corruption land with or without a second write.

My takeaway from the related discussions (e.g. RFC 1236, #27719, the RecoverSafe trait) was that only unsafe code and Drop impls should have to worry about panic safety.

This is not quite the case - AssertRecoverSafe and thread locals expand that surface area.

What's specifically the expected behavior here? Is the Drop implementation specifically the thing that needs tweaking, or should the BufWriter totally poison itself?

For reference, Java's BufferedOutputStream class has no special handling for exceptions thrown from the inner OutputStream: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/io/BufferedOutputStream.java, so adjusting Drop alone seems like a reasonable route to take.

[dgrunwald]

If your Write impl is panicking in the middle of a write, it seems to me like you're in data corruption land with or without a second write.

Without the BufWriter, a panic in the middle of a write (without BufWriter) only corrupts the part of the file that the application meant to overwrite. In a DB engine context, that means the resulting corruption can be repaired when the transaction is rolled back.

But with BufWriter, a panic after some data was written might corrupt parts of the file that are not saved in the rollback journal, so recovery not possible.

What's specifically the expected behavior here? Is the Drop implementation specifically the thing that needs tweaking, or should the BufWriter totally poison itself?

I think we need some guideline like: If a method call panics, it's a bug to call any additional methods on the same object.
Thus, the bug is in impl Drop for BufWriter.
BufWriter may need something similar to a poison-flag so that the Drop impl can detect whether a previous call panicked, but it doesn't need to protect against it's own methods being called after a panic (that would be considered a bug in the caller).

The Java BufferedOutputStream has the same problem, but the expectation in the Java world is different: it's trivially possible to catch an exception and continue using the potentially-invalid state; so in Java's case I would argue that all OutputStream implementations should provide the strong exception safety guarantee to avoid this kind of issue.

Essentially, the Rust world needs to decide between a Java/C++-style "strong exception safety guarantee" (=failing method call should not have any side effects) or a new "no-use-after-panic guarantee" (=after a failing method call, no additional method calls should occur).

Some types (like BufWriter) can be compatible with at most one of the guarantees -- strong exception safety requires that a panicking inner.write call has no side effects; no-use-after-panic requires that a panicking inner.write call has the side effect of suppressing the buffer flush in the Drop impl.

The RecoverSafe trait seems to be roughly equivalent to basic exception safety. Types that do not implement RecoverSafe instead live in the no-use-after-panic world. Generic types implementing Drop need to be careful not to call any functions on the type parameter from within the Drop implementation if a previous call panicked (unless they use a T: RecoverSafe bound).

[alexcrichton]

triage: P-medium

@sfackler is fixing the data corruption in #31068