Fix channel closing race condition bug#803
Open
adfoster-r7 wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR targets a Windows-specific stability/correctness issue in Meterpreter’s channel/socket handling, and makes the build Rake tasks fail fast when underlying commands fail.
Changes:
- Make
win_compileandjava_compileabort the Rake run whenmake.bat/mvnfails. - Fix
channel_writeto sendTLV_TYPE_LENGTHas the actual buffer length (instead of the channel id). - Add additional TCP/channel debug logging and mitigate a TCP client close race by nulling the channel pointer before teardown.
Impact Analysis:
- Blast radius: high; the
channel_writeTLV payload is part of the core Meterpreter wire contract and affects any consumer interpreting write acknowledgements, plus TCP channel teardown on Windows (direct) and any features built atop it (transitive). - Data and contract effects:
TLV_TYPE_LENGTHnow correctly represents byte length, changing on-the-wire semantics from incorrect to correct; this should fix downstream parsing/logic that relies onTLV_TYPE_LENGTHfor writes. - Rollback and test focus: rollback is straightforward (code-only), but validation should focus on Windows TCP channel open/write/close flows, write completion callbacks that consume
TLV_TYPE_LENGTH, and repeated connect/disconnect scenarios that previously reproduced the race.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| gem/Rakefile | Abort Rake tasks when Windows/Java build commands fail. |
| c/meterpreter/source/metsrv/channel.c | Fix TLV_TYPE_LENGTH in channel_write and add channel-close debug logging. |
| c/meterpreter/source/extensions/stdapi/server/net/socket/tcp.c | Add logging and reduce a channel teardown race by nulling ctx->channel earlier. |
| c/meterpreter/source/extensions/stdapi/server/net/socket/tcp_server.c | Add additional server-side socket/channel debug logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bfd4b61 to
67c91da
Compare
67c91da to
ba7256a
Compare
| TcpClientContext *ctx = (TcpClientContext *)context; | ||
|
|
||
| dprintf( "[TCP] tcp_channel_client_close. channel=0x%08X, ctx=0x%08X", channel, ctx ); | ||
| dprintf("[TCP] tcp_channel_client_close. channel=0x%08X id=%u ctx=0x%08X", channel, met_api->channel.get_id(channel), ctx); |
9aff7cf to
535842d
Compare
535842d to
181a8b6
Compare
adfoster-r7
commented
Jul 10, 2026
| // Sleep for a quarter second | ||
| // Sleep briefly to let the framework drain buffered channel data | ||
| // before we tear down the underlying socket. | ||
| Sleep(250); |
Contributor
Author
There was a problem hiding this comment.
I think this is redundant, but I'll leave it in for now
adfoster-r7
commented
Jul 10, 2026
| } | ||
|
|
||
| packet_add_tlv_uint(request, TLV_TYPE_LENGTH, channel_get_id(channel)); | ||
| packet_add_tlv_uint(request, TLV_TYPE_LENGTH, length); |
adfoster-r7
commented
Jul 10, 2026
|
|
||
| task :win_compile do | ||
| Dir.chdir(c_source) do | ||
| system('cmd.exe /c make.bat') |
Contributor
Author
There was a problem hiding this comment.
when there were issues during compilation, we'd just silently continue and ship the broken gem
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem:
The windows meterpreter
test/socket_channelstests would fail 90% of the time - either freezing indefinitely, or sending wrong use-after-free values back to msfconsole for channel closes - i.e. the close channel id was expected to be 4, but it could be1960858576in some scenariosFixes:
(recv() == 0)core_channel_closerequest from the framework, which callschannel_destroy -> tcp_channel_client_closeTests: