Skip to content

Small PR to test new review process [do not merge] #2136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
adamdossa wants to merge 1 commit into from
Closed

Small PR to test new review process [do not merge] #2136

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 32 additions & 17 deletions eth/api_admin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,39 +42,54 @@ func NewAdminAPI(eth *Ethereum) *AdminAPI {

// ExportChain exports the current blockchain into a local file,
// or a range of blocks if first and last are non-nil.
func (api *AdminAPI) ExportChain(file string, first *uint64, last *uint64) (bool, error) {
func (api *AdminAPI) ExportChain(file string, first, last *uint64) (bool, error) {
// Validate input: last cannot be specified without first
if first == nil && last != nil {
return false, errors.New("last cannot be specified without first")
}
if first != nil && last == nil {
head := api.eth.BlockChain().CurrentHeader().Number.Uint64()
last = &head

// If first is provided but last isn't, use current chain head as last
var exportFirst, exportLast *uint64
if first != nil {
exportFirst = first
if last != nil {
exportLast = last
} else {
head := api.eth.BlockChain().CurrentHeader().Number.Uint64()
exportLast = &head
}
}

// Prevent overwriting an existing file
if _, err := os.Stat(file); err == nil {
// File already exists. Allowing overwrite could be a DoS vector,
// since the 'file' may point to arbitrary paths on the drive.
return false, errors.New("location would overwrite an existing file")
}
// Make sure we can create the file to export into

// Defer cleanup and resource closure
out, err := os.OpenFile(file, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0644)
if err != nil {
return false, err
}
defer out.Close()

Comment on lines 66 to 74
Copy link

@claude claude bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing: ExportChain has a TOCTOU race between os.Stat (line 66) and os.OpenFile with O_CREATE|O_TRUNC (line 71) -- another process could create the file in between, and O_TRUNC would silently overwrite it. Additionally, if the export fails after the file is created, the stale file is never cleaned up, making retries permanently fail with "location would overwrite an existing file". Both issues existed before this PR, but the refactoring touched this exact code and could have fixed them. Replace the Stat check and O_TRUNC with os.O_EXCL (atomic create-or-fail), and add a deferred os.Remove on error.

Extended reasoning...

TOCTOU Race Condition

The code at lines 66-71 first checks whether a file exists using os.Stat(file), then opens it with os.OpenFile(file, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0644). Between these two calls, another process could create a file at the same path. Because O_TRUNC is used, OpenFile would silently overwrite the newly-created file rather than failing. This is the classic Time-of-Check-to-Time-of-Use (TOCTOU) pattern.

The old code actually had a comment warning about this exact DoS vector: "File already exists. Allowing overwrite could be a DoS vector, since the file may point to arbitrary paths on the drive." This PR removed that comment during refactoring, reducing future developer awareness of the risk. The file parameter comes from an RPC caller and can point to arbitrary filesystem paths.

Missing Error Cleanup

If ExportN or Export fails after os.OpenFile has already created the file, the function returns (false, error) but never deletes the partially-written file. On a retry attempt, the os.Stat check finds the leftover file and returns "location would overwrite an existing file", permanently blocking retries until the user manually deletes the file.

Step-by-step proof:

  1. User calls ExportChain("dump.gz", first, last)
  2. os.Stat("dump.gz") returns an error (file does not exist) -- check passes
  3. os.OpenFile creates dump.gz successfully
  4. ExportN fails midway (e.g., a block is missing from the chain)
  5. Function returns (false, exportErr) -- defer out.Close() runs but no os.Remove
  6. dump.gz remains on disk as a partial/corrupt file
  7. User retries ExportChain("dump.gz", first, last)
  8. os.Stat("dump.gz") succeeds (file exists from step 3) -> returns "location would overwrite an existing file"
  9. Export is permanently stuck without manual file deletion

The comment added at line 70 -- "Defer cleanup and resource closure" -- is misleading because no actual file cleanup (deletion) occurs on error; only the file handle is closed.

Recommended Fix

Replace both the os.Stat check and O_TRUNC with os.O_EXCL, which atomically fails if the file already exists:

out, err := os.OpenFile(file, os.O_CREATE|os.O_WRONLY|os.O_EXCL, 0644)

This eliminates the race condition and the need for the separate Stat check entirely. Additionally, add a deferred cleanup to remove the file on export error:

defer func() {
    out.Close()
    if exportErr \!= nil {
        os.Remove(file)
    }
}()

Both issues are pre-existing (the old code had the identical patterns), but since this PR directly refactors the affected code, it is a natural opportunity to address them.

var writer io.Writer = out
// Handle gzip compression transparently
writer := io.Writer(out)
var gzWriter *gzip.Writer
if strings.HasSuffix(file, ".gz") {
writer = gzip.NewWriter(writer)
defer writer.(*gzip.Writer).Close()
gzWriter = gzip.NewWriter(out)
writer = gzWriter
defer gzWriter.Close()
}

Comment on lines +80 to 83
Copy link

@claude claude bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing issue: defer gzWriter.Close() discards the error from gzip.Writer.Close(), which flushes remaining compressed data and writes the GZIP footer. If this flush fails (e.g., disk full), ExportChain returns (true, nil) indicating success while the .gz file is actually corrupt/truncated. Consider closing the gzip writer explicitly before the success return and checking its error.

Extended reasoning...

What the bug is

gzip.Writer.Close() is not just a resource cleanup call — it flushes any remaining compressed data from internal buffers and writes the mandatory GZIP footer (CRC32 checksum and size fields). If this operation fails (due to disk full, I/O error, etc.), the error is silently discarded by defer gzWriter.Close() on line 83. The function then proceeds to return (true, nil) on line 93, telling the caller the export succeeded.

Step-by-step proof

  1. User calls ExportChain("chain.gz", nil, nil)
  2. File is created, gzWriter is initialized (line 82), and defer gzWriter.Close() is registered (line 83)
  3. api.eth.BlockChain().Export(writer) succeeds — exportErr is nil
  4. The function reaches line 93: return true, nil — caller is told export succeeded
  5. Deferred gzWriter.Close() runs, attempting to flush remaining compressed data and write the GZIP footer
  6. The write fails (e.g., disk became full during the deferred close)
  7. The error from Close() is discarded — nobody checks it
  8. The .gz file on disk is missing its GZIP footer, making it corrupt — gunzip will report an error

Why existing code doesn't prevent it

The defer statement on line 83 calls gzWriter.Close() but does not capture or propagate the returned error. This is a common Go pattern for read cleanup (where close errors are usually benign), but for write operations — especially gzip, which performs meaningful work on Close — the error is significant.

Pre-existing nature

The old code had the identical issue: defer writer.(*gzip.Writer).Close() also discarded the error. This PR refactored the gzip handling (introducing the named gzWriter variable), which was an opportunity to fix this, but the error-ignoring behavior was carried over unchanged. The pattern also appears in other places in the codebase (e.g., cmd/utils/cmd.go).

How to fix

Close the gzip writer explicitly before the success return and check its error:

if exportErr != nil {
    return false, exportErr
}
if gzWriter != nil {
    if err := gzWriter.Close(); err != nil {
        return false, err
    }
}
return true, nil

The defer gzWriter.Close() can be kept as a safety net (it is a no-op if already closed), or removed entirely since the explicit close covers the success path and the error paths don't need a valid gzip footer anyway.

// Export the blockchain
if first != nil {
if err := api.eth.BlockChain().ExportN(writer, *first, *last); err != nil {
return false, err
}
} else if err := api.eth.BlockChain().Export(writer); err != nil {
return false, err
// Export blockchain data
var exportErr error
if exportFirst != nil {
exportErr = api.eth.BlockChain().ExportN(writer, *exportFirst, *exportLast)
} else {
exportErr = api.eth.BlockChain().Export(writer)
}
if exportErr != nil {
return false, exportErr
}
return true, nil
}
Expand Down
Loading