Skip to content

internal/download: close dst on io.Copy error#34910

Merged
fjl merged 1 commit into
ethereum:masterfrom
rayjun:fix/download-fd-leak-io-copy-error
May 10, 2026
Merged

internal/download: close dst on io.Copy error#34910
fjl merged 1 commit into
ethereum:masterfrom
rayjun:fix/download-fd-leak-io-copy-error

Conversation

@rayjun
Copy link
Copy Markdown
Contributor

@rayjun rayjun commented May 8, 2026

PR #34866 refactored DownloadFile to return early on io.Copy errors, but moved dst.Close() behind the error check. When io.Copy fails (e.g. on a mid-download network error), the underlying file descriptor is no longer closed before the function returns - only os.Remove is called against the still-open tmpfile, which is a no-op on Windows.

Before #34866:

_, err = io.Copy(dst, resp.Body)
dst.Close()              // unconditional
if err != nil {
    os.Remove(tmpfile)
    return err
}

After #34866:

if _, err = io.Copy(dst, resp.Body); err != nil {
    os.Remove(tmpfile)   // dst never closed
    return err
}
if err = dst.Close(); err != nil {
    ...
}

Restore the pre-refactor invariant by closing dst on the io.Copy error path before os.Remove, matching the existing close-then-remove ordering on the dst.Close() error path a few lines below.

PR ethereum#34866 refactored DownloadFile to return early on io.Copy errors, but
moved dst.Close() behind the error check. When io.Copy fails (e.g. on a
mid-download network error), the underlying file descriptor is no longer
closed before the function returns - only os.Remove is called against the
still-open tmpfile, which is a no-op on Windows.

Before ethereum#34866:

    _, err = io.Copy(dst, resp.Body)
    dst.Close()              // unconditional
    if err != nil {
        os.Remove(tmpfile)
        return err
    }

After ethereum#34866:

    if _, err = io.Copy(dst, resp.Body); err != nil {
        os.Remove(tmpfile)   // dst never closed
        return err
    }
    if err = dst.Close(); err != nil {
        ...
    }

Restore the pre-refactor invariant by closing dst on the io.Copy error
path before os.Remove, matching the existing close-then-remove ordering
on the dst.Close() error path a few lines below.
@fjl fjl merged commit f63c265 into ethereum:master May 10, 2026
6 of 9 checks passed
@fjl fjl added this to the 1.17.3 milestone May 10, 2026
@rayjun rayjun deleted the fix/download-fd-leak-io-copy-error branch May 11, 2026 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants