microsoft / microsoft/dev-tunnels-ssh

Using `wantReply` on an `ChannelSignalMessage` can lead to deadlock

Open
#110 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
161
Forks
34
Avg merge
1d 11h
Merged PRs (30d)
2

Description

In both the C# and TS implementations, the ChannelSignalMessage explicitly sets wantReply to false before sending:

https://github.com/microsoft/dev-tunnels-ssh/blob/1cd94f90ee5b9cc55120d050a499af094dbd0b8e/src/ts/ssh/messages/connectionMessages.ts#L403

However, this change is done while the message is being written to binary, after the SshChannel checks the value:

https://github.com/microsoft/dev-tunnels-ssh/blob/1cd94f90ee5b9cc55120d050a499af094dbd0b8e/src/ts/ssh/sshChannel.ts#L201

This means that if you set wantReply on the message, the SshChannel will wait for a response which will never come, because it wasn't actually asked for.

This is easy to reproduce:

const signalMessage = new ChannelSignalMessage();
signalMessage.signal = 'INT';
signalMessage.wantReply = true;

// get a channel...

const res = await channel.request(signalMessage); // Deadlock!

There's no comment on the line that overrides the wantReply value so I'm not sure the purpose of it - reading the OpenSSH source it seems like wantReply is valid for a signal message and would be handled.

If this is intentional, it would probably just be best to remove the wantReply property from the ChannelSignalMessage. Otherwise, it would be good to remove the override.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with src/ts/ssh/messages/connectionMessages.ts around ChannelSignalMessage and src/ts/ssh/sshChannel.ts around the request check, then locate the corresponding C# implementation. Reproduce the wantReply=true example and verify that the chosen behavior no longer leaves channel.request waiting for a response that was not requested.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp, typescript
Domain
networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.