Skip to content

Return an error instead of crashing when a Writer is used after Close - #175

Open
vignesh2027 wants to merge 1 commit into
DataDog:1.xfrom
vignesh2027:fix-writer-use-after-close
Open

vignesh2027 wants to merge 1 commit into
DataDog:1.xfrom
vignesh2027:fix-writer-use-after-close

Conversation

@vignesh2027

Copy link
Copy Markdown

Calling Write, Flush or SetNbWorkers on a Writer after Close crashes the whole process instead of returning an error.

Since #163, Close frees the C compression stream and sets w.ctx to nil, which fixed the leak and made double Close safe. The other Writer methods do not check for that, so they pass a nil ZSTD_CCtx to libzstd and the program dies with a SIGSEGV inside cgo. Because it happens in C code, recover cannot catch it.

var buf bytes.Buffer
w := zstd.NewWriter(&buf)
w.Write([]byte("hello"))
w.Close()
w.Write([]byte("again")) // SIGSEGV, signal arrived during cgo execution

The same thing happens after a Close that fails because the underlying writer returned an error, since that path also nils ctx without setting firstError.

This change makes Write, Flush and SetNbWorkers return a new errWriterClosed when ctx is nil, the same way reader.Read already returns errReaderClosed after Close. An earlier firstError is still returned first, and a second Close still returns nil, so the existing tests from #163 keep passing.

I added two tests next to the #163 ones. On main both crash the test binary with SIGSEGV in ZSTD_compressStream2_wrapper, and with the fix they pass. I ran go test with the CI payload, go test -race, and the suite with -tags external_libzstd against a system libzstd, all green.

Close frees the C compression stream and sets ctx to nil, but Write,
Flush and SetNbWorkers still passed that nil ctx to libzstd. Calling any
of them after Close (or after a Close that failed on the underlying
writer) crashed the process with SIGSEGV in cgo instead of returning an
error. They now return errWriterClosed, the same way reader.Read returns
errReaderClosed.
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.

1 participant