Skip to content

Commit cdaf9c0

Browse files
committed
Fix duplicate character echoing in ProcessShell
- Remove redundant manual ECHO handling in TtyFilterOutputStream - Update ProcessShell to stop passing the echo stream to TtyFilterOutputStream - Update TtyFilterOutputStream constructors to remove the unused echo parameter - Update tests to match the new TtyFilterOutputStream API This fix prevents characters from being duplicated in interactive shell sessions where the underlying shell process already handles echoing.
1 parent 5c0a7b8 commit cdaf9c0

4 files changed

Lines changed: 123 additions & 16 deletions

File tree

sshd-common/src/main/java/org/apache/sshd/server/shell/TtyFilterOutputStream.java

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -25,33 +25,30 @@
2525
import java.util.Collections;
2626
import java.util.EnumSet;
2727
import java.util.Map;
28-
import java.util.Objects;
2928
import java.util.Set;
3029

3130
import org.apache.sshd.common.channel.PtyMode;
3231
import org.apache.sshd.common.util.GenericUtils;
3332

3433
/**
35-
* Handles the output stream while taking care of the {@link PtyMode} for CR / LF and ECHO settings
34+
* Handles the output stream while taking care of the {@link PtyMode} for CR / LF settings
3635
*
3736
* @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
3837
*/
3938
public class TtyFilterOutputStream extends FilterOutputStream {
4039
public static final Set<PtyMode> OUTPUT_OPTIONS
41-
= Collections.unmodifiableSet(EnumSet.of(PtyMode.ECHO, PtyMode.INLCR, PtyMode.ICRNL, PtyMode.IGNCR));
40+
= Collections.unmodifiableSet(EnumSet.of(PtyMode.INLCR, PtyMode.ICRNL, PtyMode.IGNCR));
4241

4342
private final Set<PtyMode> ttyOptions;
44-
private final TtyFilterInputStream echo;
4543

46-
public TtyFilterOutputStream(OutputStream out, TtyFilterInputStream echo, Map<PtyMode, ?> modes) {
47-
this(out, echo, PtyMode.resolveEnabledOptions(modes, OUTPUT_OPTIONS));
44+
public TtyFilterOutputStream(OutputStream out, Map<PtyMode, ?> modes) {
45+
this(out, PtyMode.resolveEnabledOptions(modes, OUTPUT_OPTIONS));
4846
}
4947

50-
public TtyFilterOutputStream(OutputStream out, TtyFilterInputStream echo, Collection<PtyMode> ttyOptions) {
48+
public TtyFilterOutputStream(OutputStream out, Collection<PtyMode> ttyOptions) {
5149
super(out);
5250
// we create a copy of the options so as to avoid concurrent modifications
5351
this.ttyOptions = GenericUtils.of(ttyOptions); // TODO validate non-conflicting options
54-
this.echo = this.ttyOptions.contains(PtyMode.ECHO) ? Objects.requireNonNull(echo, "No echo stream") : echo;
5552
}
5653

5754
@Override
@@ -86,9 +83,6 @@ protected void handleLF() throws IOException {
8683

8784
protected void writeRawOutput(int c) throws IOException {
8885
this.out.write(c);
89-
if (ttyOptions.contains(PtyMode.ECHO)) {
90-
echo.write(c);
91-
}
9286
}
9387

9488
@Override
@@ -119,8 +113,5 @@ public void write(byte[] b, int off, int len) throws IOException {
119113

120114
protected void writeRawOutput(byte[] b, int off, int len) throws IOException {
121115
this.out.write(b, off, len);
122-
if (ttyOptions.contains(PtyMode.ECHO)) {
123-
echo.write(b, off, len);
124-
}
125116
}
126117
}

sshd-common/src/test/java/org/apache/sshd/server/shell/TtyFilterOutputStreamTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ public void write(int b) throws IOException {
8484
}
8585
};
8686
TtyFilterOutputStream ttyOut = new TtyFilterOutputStream(
87-
output, null, PtyMode.ECHO.equals(mode) ? Collections.emptySet() : EnumSet.of(mode));
87+
output, PtyMode.ECHO.equals(mode) ? Collections.emptySet() : EnumSet.of(mode));
8888
Writer writer = new OutputStreamWriter(ttyOut, StandardCharsets.UTF_8)) {
8989

9090
for (String l : lines) {

sshd-core/src/main/java/org/apache/sshd/server/shell/ProcessShell.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ public void start(ChannelSession channel, Environment env) throws IOException {
121121
Map<PtyMode, ?> modes = resolveShellTtyOptions(env.getPtyModes());
122122
out = new TtyFilterInputStream(process.getInputStream(), modes);
123123
err = new TtyFilterInputStream(process.getErrorStream(), modes);
124-
in = new TtyFilterOutputStream(process.getOutputStream(), err, modes);
124+
in = new TtyFilterOutputStream(process.getOutputStream(), modes);
125125
}
126126

127127
protected Map<String, String> resolveShellEnvironment(Map<String, String> env) {
Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.apache.sshd.server.shell;
20+
21+
import java.io.ByteArrayOutputStream;
22+
import java.io.OutputStream;
23+
import java.nio.charset.StandardCharsets;
24+
25+
import org.apache.sshd.client.SshClient;
26+
import org.apache.sshd.client.channel.ChannelShell;
27+
import org.apache.sshd.client.session.ClientSession;
28+
import org.apache.sshd.common.util.OsUtils;
29+
import org.apache.sshd.server.SshServer;
30+
import org.apache.sshd.util.test.BaseTestSupport;
31+
import org.apache.sshd.util.test.CoreTestSupportUtils;
32+
import org.junit.jupiter.api.AfterEach;
33+
import org.junit.jupiter.api.BeforeEach;
34+
import org.junit.jupiter.api.Test;
35+
36+
public class ProcessShellTest extends BaseTestSupport {
37+
private SshClient client;
38+
39+
public ProcessShellTest() {
40+
super();
41+
}
42+
43+
@BeforeEach
44+
public void setUp() throws Exception {
45+
client = CoreTestSupportUtils.setupTestClient(getClass());
46+
client.start();
47+
}
48+
49+
@AfterEach
50+
public void tearDown() throws Exception {
51+
if (client != null) {
52+
client.stop();
53+
}
54+
}
55+
56+
/**
57+
* Verifies that characters sent to the shell are returned exactly as they were sent, without any modifications or
58+
* redundant echoes from the server.
59+
* <p>
60+
* This test ensures that the SSH server does not inject extra characters (like manual echoes) into the data stream.
61+
* It verifies that characters sent to the shell are returned without any modifications. If the server were to
62+
* return modified characters or extra echoes, it would indicate a failure in the I/O bridging logic.
63+
* </p>
64+
* <p>
65+
* The test relies on {@code cat} as a shell because it is a simple, standard Linux utility that acts as a
66+
* transparent bridge, reading from STDIN and writing to STDOUT without any internal processing, echoing, or
67+
* modifications to the character stream. This allows the test to isolate and verify the server's behavior without
68+
* interference from the shell process itself.
69+
* </p>
70+
* <p>
71+
* Note: This test is skipped on Windows because {@code cat} is not natively available. Since the core logic being
72+
* tested is platform-independent Java, verifying it on Unix-like systems is sufficient.
73+
* </p>
74+
*
75+
* @throws Exception If failed
76+
*/
77+
@Test
78+
void testNoRedundantEchoOnStderr() throws Exception {
79+
if (OsUtils.isWin32()) {
80+
return;
81+
}
82+
83+
try (SshServer localSshd = CoreTestSupportUtils.setupTestServer(getClass())) {
84+
localSshd.setShellFactory(new ProcessShellFactory("cat", "cat"));
85+
localSshd.start();
86+
87+
try (ClientSession session = client.connect(getCurrentTestName(), "localhost", localSshd.getPort())
88+
.verify(10_000).getSession()) {
89+
session.addPasswordIdentity(getCurrentTestName());
90+
session.auth().verify(10_000);
91+
92+
try (ChannelShell channel = session.createShellChannel();
93+
ByteArrayOutputStream out = new ByteArrayOutputStream();
94+
ByteArrayOutputStream err = new ByteArrayOutputStream()) {
95+
channel.setOut(out);
96+
channel.setErr(err);
97+
channel.open().verify(10_000);
98+
99+
OutputStream pipe = channel.getInvertedIn();
100+
pipe.write("hello\n".getBytes(StandardCharsets.UTF_8));
101+
pipe.flush();
102+
103+
Thread.sleep(1000);
104+
105+
String stderr = err.toString(StandardCharsets.UTF_8.name());
106+
assertFalse(stderr.contains("hello"), "Redundant echo detected on stderr: " + stderr);
107+
108+
String stdout = out.toString(StandardCharsets.UTF_8.name());
109+
assertTrue(stdout.contains("hello"), "Output should contain 'hello': " + stdout);
110+
}
111+
} finally {
112+
localSshd.stop(true);
113+
}
114+
}
115+
}
116+
}

0 commit comments

Comments
 (0)