Skip to content

fix: close tmp key file in fuzz test (ethereum#29444) - #125

Open
hmlee-wm wants to merge 1 commit into
devfrom
fix/signify-fuzz-close-tmp-key-file
Open

hmlee-wm wants to merge 1 commit into
devfrom
fix/signify-fuzz-close-tmp-key-file

Conversation

@hmlee-wm

@hmlee-wm hmlee-wm commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Ports ethereum#29444. createKeyPair() opened a temp file via os.CreateTemp but never closed it. Other temp files in the same file (tmpFile, pubKeyFile) already have defer Close(). Add the missing defer tmpKey.Close().

Upstream References

PR Description
#29444 crypto/signify: close tmp key file in test

Changes

  • crypto/signify/signify_fuzz.go: add defer tmpKey.Close() in createKeyPair()

(cherry picked from commit 8bd03341689c992d633f3988b3a7fbc15aec75e6)
@hmlee-wm hmlee-wm self-assigned this Sep 4, 2026
@hmlee-wm hmlee-wm added the bug Something isn't working label Sep 4, 2026

@0xmhha 0xmhha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note: the fuzz harness in this file cannot be built, so the change is unverifiable

No impact on the normal build or make testsignify_fuzz.go carries
//go:build gofuzz, so it is excluded from both. go build ./crypto/signify/
passes. The fuzzer is also not registered in oss-fuzz.sh, so nothing in CI
builds it either.

The problem only surfaces when the harness is built with the tag:

$ go build -tags gofuzz ./crypto/signify/
# github.com/ethereum/go-ethereum/crypto/signify
crypto/signify/signify_fuzz.go:59:8: undefined: SignifySignFile
crypto/signify/signify_fuzz.go:71:9: no new variables on left side of :=

go vet reports the same, but stops at the first error — worth knowing, since it
makes the problem look like a single-line fix when it is not:

$ go vet -tags gofuzz ./crypto/signify/
# github.com/ethereum/go-ethereum/crypto/signify
# [github.com/ethereum/go-ethereum/crypto/signify]
vet: crypto/signify/signify_fuzz.go:59:8: undefined: SignifySignFile

Both errors are pre-existing and reproduce on dev:

  1. L59 — SignifySignFile was renamed to SignFile in f935b1d
    (ethereum#21977); the call site was never updated. It is the only remaining
    reference in the tree.
  2. L71 — _, err := exec.LookPath(signify); err is already declared at L37
    and _ is not a new variable, so := has nothing new to declare.

Upstream fixed both in ethereum#33402 (a9eaf2ffd859, 2025-12-13). Applying that
patch locally makes go build -tags gofuzz ./crypto/signify/ pass.

The change in this PR is correct on its own: defer tmpKey.Close() is registered
after the three os.Remove defers, so LIFO order closes the handle before the
files are removed.

@colinkim colinkim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.
Since the gofuzz-tagged harness build failure predates this PR, and CI does not exercise that tag, how about tracking it in a separate follow-up issue or PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants