Skip to content

Commit 92b6157

Browse files
daniel-beckjenkinsci-cert-ci
daniel-beck
authored and
jenkinsci-cert-ci
committed
1 parent 20bc304 commit 92b6157

File tree

4 files changed

+154
-8
lines changed

4 files changed

+154
-8
lines changed

‎cli/src/main/java/hudson/cli/PlainCLIProtocol.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,15 +153,14 @@ public void run() {
153153
}
154154
} catch (ClosedChannelException x) {
155155
LOGGER.log(Level.FINE, null, x);
156-
side.handleClose();
157156
} catch (IOException x) {
158157
LOGGER.log(Level.WARNING, null, flightRecorder.analyzeCrash(x, "broken stream"));
159158
} catch (ReadPendingException x) {
160159
// in case trick in CLIAction does not work
161160
LOGGER.log(Level.FINE, null, x);
162-
side.handleClose();
163161
} catch (RuntimeException x) {
164162
LOGGER.log(Level.WARNING, null, x);
163+
} finally {
165164
side.handleClose();
166165
}
167166
}

‎core/src/main/java/hudson/cli/CLIAction.java‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,11 +40,11 @@
4040
import java.nio.charset.Charset;
4141
import java.nio.charset.UnsupportedCharsetException;
4242
import java.util.ArrayList;
43-
import java.util.HashMap;
4443
import java.util.List;
4544
import java.util.Locale;
4645
import java.util.Map;
4746
import java.util.UUID;
47+
import java.util.concurrent.ConcurrentHashMap;
4848
import java.util.logging.Level;
4949
import java.util.logging.Logger;
5050
import jenkins.model.Jenkins;
@@ -80,7 +80,7 @@ public class CLIAction implements UnprotectedRootAction, StaplerProxy {
8080
*/
8181
/* package-private for testing */ static /* non-final for Script Console */ Boolean ALLOW_WEBSOCKET = SystemProperties.optBoolean(CLIAction.class.getName() + ".ALLOW_WEBSOCKET");
8282

83-
private final transient Map<UUID, FullDuplexHttpService> duplexServices = new HashMap<>();
83+
private final transient Map<UUID, FullDuplexHttpService> duplexServices = new ConcurrentHashMap<>();
8484

8585
@Override
8686
public String getIconFileName() {
@@ -315,8 +315,13 @@ private synchronized void ready() {
315315

316316
void run() throws IOException, InterruptedException {
317317
synchronized (this) {
318-
while (!ready) {
319-
wait();
318+
long end = System.currentTimeMillis() + FullDuplexHttpService.CONNECTION_TIMEOUT;
319+
while (!ready && System.currentTimeMillis() < end) {
320+
wait(1000);
321+
}
322+
if (!ready) {
323+
LOGGER.log(Level.FINE, "CLI timeout waiting for client");
324+
return;
320325
}
321326
}
322327
PrintStream stdout = new PrintStream(streamStdout(), false, encoding);

‎core/src/main/java/jenkins/util/FullDuplexHttpService.java‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,8 +142,15 @@ public synchronized void upload(StaplerRequest2 req, StaplerResponse2 rsp) throw
142142
notify();
143143

144144
// wait until we are done
145-
while (!completed) {
146-
wait();
145+
long end = System.currentTimeMillis() + CONNECTION_TIMEOUT;
146+
while (!completed && System.currentTimeMillis() < end) {
147+
LOGGER.log(Level.FINE, "Waiting for upload stream {0} for {1}: {2}", new Object[] {upload, uuid, this});
148+
wait(1000);
149+
}
150+
151+
if (!completed) {
152+
LOGGER.log(Level.FINE, "Timeout reached for {0} for {1}: {2}", new Object[] {upload, uuid, this});
153+
throw new IOException("CLI timeout");
147154
}
148155
}
149156

‎test/src/test/java/hudson/cli/Security3630Test.java‎

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
package hudson.cli;
2+
3+
import static org.hamcrest.MatcherAssert.assertThat;
4+
import static org.hamcrest.Matchers.allOf;
5+
import static org.hamcrest.Matchers.containsString;
6+
import static org.hamcrest.Matchers.empty;
7+
import static org.hamcrest.Matchers.hasItem;
8+
import static org.hamcrest.Matchers.hasProperty;
9+
import static org.hamcrest.Matchers.instanceOf;
10+
import static org.hamcrest.Matchers.not;
11+
import static org.hamcrest.Matchers.nullValue;
12+
import static org.jvnet.hudson.test.LoggerRule.recorded;
13+
14+
import hudson.Functions;
15+
import hudson.init.impl.InstallUncaughtExceptionHandler;
16+
import java.io.File;
17+
import java.io.IOException;
18+
import java.lang.management.ThreadInfo;
19+
import java.net.URL;
20+
import java.util.ArrayList;
21+
import java.util.Arrays;
22+
import java.util.List;
23+
import java.util.Set;
24+
import java.util.UUID;
25+
import java.util.concurrent.Callable;
26+
import java.util.concurrent.ExecutionException;
27+
import java.util.concurrent.ExecutorService;
28+
import java.util.concurrent.Executors;
29+
import java.util.concurrent.Future;
30+
import java.util.concurrent.TimeUnit;
31+
import java.util.logging.Level;
32+
import java.util.stream.Collectors;
33+
import jenkins.util.FullDuplexHttpService;
34+
import org.apache.commons.io.FileUtils;
35+
import org.awaitility.Awaitility;
36+
import org.htmlunit.HttpMethod;
37+
import org.htmlunit.WebRequest;
38+
import org.junit.jupiter.api.AfterEach;
39+
import org.junit.jupiter.api.BeforeEach;
40+
import org.junit.jupiter.api.Test;
41+
import org.junit.jupiter.api.io.TempDir;
42+
import org.junit.platform.commons.util.ExceptionUtils;
43+
import org.jvnet.hudson.test.JenkinsRule;
44+
import org.jvnet.hudson.test.LoggerRule;
45+
import org.jvnet.hudson.test.junit.jupiter.WithJenkins;
46+
47+
@WithJenkins
48+
public class Security3630Test {
49+
public static final int CONCURRENCY = 50;
50+
public static final int ITERATIONS = 50;
51+
private JenkinsRule j = new JenkinsRule();
52+
53+
@TempDir
54+
private File tmp;
55+
56+
private LoggerRule loggerRule = new LoggerRule().record(InstallUncaughtExceptionHandler.class.getName(), Level.WARNING);
57+
58+
private long originalTimeout;
59+
60+
@BeforeEach
61+
public void setup(JenkinsRule j) throws Exception {
62+
this.j = j;
63+
// TODO FlagRule in JUnit 5?
64+
originalTimeout = FullDuplexHttpService.CONNECTION_TIMEOUT;
65+
FullDuplexHttpService.CONNECTION_TIMEOUT = TimeUnit.SECONDS.toMillis(5);
66+
}
67+
68+
@AfterEach
69+
public void reset() {
70+
FullDuplexHttpService.CONNECTION_TIMEOUT = originalTimeout;
71+
}
72+
73+
@Test
74+
void control() throws IOException {
75+
loggerRule.capture(100);
76+
final String uuid = UUID.randomUUID().toString();
77+
try (JenkinsRule.WebClient wc = j.createWebClient()) {
78+
WebRequest request = new WebRequest(new URL(j.getURL().toString() + "cli?remoting=false"));
79+
request.setHttpMethod(HttpMethod.POST);
80+
request.setAdditionalHeader("Session", uuid);
81+
request.setAdditionalHeader("Side", "download");
82+
wc.getPage(request);
83+
}
84+
assertThat(loggerRule, recorded(nullValue(String.class), allOf(instanceOf(IOException.class), hasProperty("message", containsString("HTTP full-duplex channel timeout: " + uuid)))));
85+
}
86+
87+
@Test
88+
void testHashMap() throws InterruptedException, IOException {
89+
// If this test appears flaky, it's probably not: The race condition cannot be reliably triggered.
90+
// If the assertion fails, then there's probably a bug here.
91+
// TODO Do we want to keep a test like this?
92+
loggerRule.capture(100);
93+
final File jar = File.createTempFile("jenkins-cli.jar", null, tmp);
94+
FileUtils.copyURLToFile(j.jenkins.getJnlpJars("jenkins-cli.jar").getURL(), jar);
95+
for (int i = 0; i < ITERATIONS; i++) {
96+
List<Callable<Void>> callables = new ArrayList<>();
97+
for (int c = 0; c < CONCURRENCY; c++) {
98+
callables.add(() -> {
99+
new ProcessBuilder().command("java", "-jar", jar.getAbsolutePath(), "-http", "-s", j.getURL().toString(), "who-am-i").start().waitFor();
100+
return null;
101+
});
102+
}
103+
104+
ExecutorService executor = Executors.newFixedThreadPool(CONCURRENCY);
105+
List<Future<Void>> futures = executor.invokeAll(callables);
106+
107+
futures.forEach(f -> {
108+
try {
109+
f.get();
110+
} catch (InterruptedException | ExecutionException e) {
111+
throw new RuntimeException(e);
112+
}
113+
});
114+
assertThat(loggerRule.getRecords().stream().map(r -> String.join("\n", r.getMessage(), r.getLoggerName(), r.getThrown().getMessage(), ExceptionUtils.readStackTrace(r.getThrown()))).collect(Collectors.toList()), empty());
115+
// See #control() assertion for the "expected" failure without the fix.
116+
}
117+
}
118+
119+
@Test
120+
void testIndefiniteWait() throws IOException {
121+
var stream = new hudson.cli.FullDuplexHttpStream(j.getURL(), "cli?remoting=false", null);
122+
{
123+
Set<String> threadNames = Arrays.stream(Functions.getThreadInfos()).map(ThreadInfo::getThreadName).collect(Collectors.toSet());
124+
assertThat(threadNames, hasItem(containsString("Handling POST /jenkins/cli")));
125+
}
126+
stream.getOutputStream().write(new byte[10]);
127+
stream.getOutputStream().close();
128+
stream.getInputStream().close();
129+
130+
Awaitility.await().atMost(FullDuplexHttpService.CONNECTION_TIMEOUT * 2, TimeUnit.MILLISECONDS).untilAsserted(() -> {
131+
Set<String> threadNames = Arrays.stream(Functions.getThreadInfos()).map(ThreadInfo::getThreadName).collect(Collectors.toSet());
132+
assertThat(threadNames, not(hasItem(containsString("Handling POST /jenkins/cli"))));
133+
});
134+
}
135+
}

0 commit comments

Comments
 (0)