zlib: don't throw when a write is flushed after cleanup - #1506
ArockiaRajamanickam wants to merge 1 commit into
Conversation
Protocol#_destruct() closes the zlib instances used by ZlibPacketWriter, but outbound data can still be flushed afterwards -- for example when SFTP's cleanupRequests() runs during connection teardown and a request callback synchronously issues another write. In that case ZlibPacketWriter#finalize() called into a closed Zlib instance, which threw 'Invalid Zlib instance' from an asynchronous callback where nothing could catch it, taking down the process. Ciphers already tolerate this: free() marks them dead and encrypt() returns early instead of throwing. Do the same for the compressing packet writer, so a late write is dropped rather than thrown. This matches the behaviour already seen with compression disabled, where the same late writes are silently discarded by the freed cipher. Fixes: mscdex#1178 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Another real-world path to the same crash, in case it helps this land: SFTP When the socket goes away, the in-flight READ requests are failed, It reproduces deterministically with an Uncompressed connections are unaffected: Investigated with an AI assistant; the reproduction and the verification were run locally. |
2 participants
Fixes #1178.
ZlibPacketWriter#cleanup()closed the zlib handle but leftthis._zlibpointing at it. Any outbound packet finalized after that reachedthis._zlib.writeSync(...)on a closed handle, and zlib throwsError: Invalid Zlib instancefrom there. It escapes through the socket'close'handler, so there is nothing in userland that can catch it and the process dies.The fix clears the reference in
cleanup()and returns early fromfinalize()when it is gone.Why it returns quietly rather than emitting an error
This was the part worth thinking about, and I went the opposite way from what the issue thread suggested.
Emitting an error here would fire on ordinary, successful teardown. I instrumented
finalize()and saw between 1 and 34 late writes per disconnect, even on healthy connections with compression off. Turning every clean disconnect into an'error'event would be a worse regression than the crash for well-behaved users, and it breakscommon.js's ownsetup()helper, which treats an unexpectederrorevent as a failure.The uncompressed path already handles this exact situation the same way:
crypto.js's cipher classes all return quietly when called afterfree(), andPacketWriter#finalize()has nothing to throw. So returning quietly matches the convention already in the codebase rather than inventing a new one for the compressed path.Nothing that could have reached the wire is discarded. By the time this triggers the socket is gone, the cipher is freed, and
Protocol#_onWritehas been replaced with a thrower — the packet was unsendable either way. The only question was whether it also killed the process.If you would rather it surfaced as an error, I think that belongs in
Protocol#_destruct()for every packet writer, not inzlib.jsalone. Happy to do it that way instead.Test
Added to
test/test-misc-client-server.js: a client and server that negotiatezlib, then callclient._protocol.channelData(...)from the client's'close'handler, which is the moment aftercleanup()when a queued write can still be flushed.Reverting only
lib/protocol/zlib.jsand keeping the test crashes withError: Invalid Zlib instanceat the same three frames as the issue's stack trace; with the fix it passes. It is deterministic — the real-world SFTP path only crashed about half the time, and you asked for a test as minimal as possible, so I drove the same code path directly rather than racing it.Two things to push back on if you disagree:
test-misc-client-server.jsalready does this in several places (conn._protocol.channelSuccess,client._protocol._offer,client._chanMgr._channels) andtest-sftp.jscallssftp._protocol.exitStatus(...), so it is consistent with the suite, but it is still white-box. It assertsinfo.cs.compress === 'zlib'on handshake so it cannot silently stop exercising the compressed path.cleanup()running before'close'is emitted (client.js:812-819). True today and what makes it deterministic, but if that order ever changed the test would quietly stop covering the bug.ZlibPacketReader#read()has the same shape of hazard. I could not construct a path that reaches it, so I left it alone rather than change code I could not exercise — flagging it rather than quietly fixing it.Verification notes
Tested on Node v25.9.0, macOS only; I did not run the full CI matrix.
test-integration-openssh.jsalready fails on a clean checkout in my environment because the host OpenSSH rejects an old RSA key fixture — that is unrelated and unchanged by this branch, so please don't read it as a regression.Also worth knowing: if #1491 (SFTP
cleanupRequestslooping) lands first, it would make this crash more frequent, since it increases the number of late writes during teardown.I used an AI assistant while working on this. The instrumentation counts, the negative control and the decision not to emit an error are mine.