Repository: bitcoin/bitcoin
File: src/streams.h (C/C++)

Code before:
 * Will automatically close the file when it goes out of scope if not null.
 * If you're returning the file pointer, return file.release().
 * If you need to close the file early, use file.fclose() instead of fclose(file).
 */
class AutoFile
{
protected:
    std::FILE* m_file;
    std::vector<std::byte> m_xor;
    std::optional<int64_t> m_position;

public:
    explicit AutoFile(std::FILE* file, std::vector<std::byte> data_xor={});

    ~AutoFile() { fclose(); }

    // Disallow copies
    AutoFile(const AutoFile&) = delete;
    AutoFile& operator=(const AutoFile&) = delete;

    bool feof() const { return std::feof(m_file); }

    int fclose()
    {
        if (auto rel{release()}) return std::fclose(rel);
        return 0;
    }

    /** Get wrapped FILE* with transfer of ownership.
     * @note This will invalidate the AutoFile object, and makes it the responsibility of the caller
     * of this function to clean up the returned FILE*.
     */
    std::FILE* release()
    {
        std::FILE* ret{m_file};
        m_file = nullptr;
        return ret;
    }

    /** Return true if the wrapped FILE* is nullptr, false otherwise.
     */
    bool IsNull() const { return m_file == nullptr; }

    /** Continue with a different XOR key */
    void SetXor(std::vector<std::byte> data_xor) { m_xor = data_xor; }

    /** Implementation detail, only used internally. */
    std::size_t detail_fread(std::span<std::byte> dst);

    /** Wrapper around fseek(). Will throw if seeking is not possible. */
    void seek(int64_t offset, int origin);

Review comment (bug):
not sure about unconditionally crashing the node on any failure here. This may or may not be an issue, so the call site will have to decide about this.

For example, If an RPC user dumps the mempool to an external (corrupt) USB thumb drive, a failure shouldn't crash the node, but notify the user over the RPC.

Also, I am not sure about involving logging in this low-level code. Logging may not be set up, so this may never be printed before the crash.

Also, I am not sure about the log message. It just says that there was an error, but not whose fault it was or what should be done about it.

A better solution overall may be to just call `if (m_was_written) Assume(IsNull());`. This should highlight all violating call sites during CI, because the test should be deterministic. (If there is a code path that is not deterministically tested, all hopes are lost anyway, and I don't think the solution should be to crash the users node completely and without considering the context?)

Code after:
/** Non-refcounted RAII wrapper for FILE*
 *
 * Will automatically close the file when it goes out of scope if not null.
 * If you're returning the file pointer, return file.release().
 * If you need to close the file early, use autofile.fclose() instead of fclose(underlying_FILE).
 *
 * @note If the file has been written to, then the caller must close it
 * explicitly with the `fclose()` method, check if it returns an error and treat
 * such an error as if the `write()` method failed. The OS's `fclose(3)` may
 * fail to flush to disk data that has been previously written, rendering the
 * file corrupt.
 */
class AutoFile
{
protected:
    std::FILE* m_file;
    std::vector<std::byte> m_xor;
    std::optional<int64_t> m_position;
    bool m_was_written{false};

public:
    explicit AutoFile(std::FILE* file, std::vector<std::byte> data_xor={});

    ~AutoFile()
    {
        if (m_was_written) {
            // Callers that wrote to the file must have closed it explicitly
            // with the fclose() method and checked that the close succeeded.
            // This is because here in the destructor we have no way to signal
            // errors from fclose() which, after write, could mean the file is
            // corrupted and must be handled properly at the call site.
            // Destructors in C++ cannot signal an error to the callers because
            // they do not return a value and are not allowed to throw exceptions.
            Assume(IsNull());
        }

        if (fclose() != 0) {
            LogError("Failed to close file: %s", SysErrorString(errno));
        }
    }

    // Disallow copies
    AutoFile(const AutoFile&) = delete;
    AutoFile& operator=(const AutoFile&) = delete;

    bool feof() const { return std::feof(m_file); }

    [[nodiscard]] int fclose()
    {
        if (auto rel{release()}) return std::fclose(rel);
        return 0;