From 1acfcb937d92b6cc0ccbf5017aae7c5d5ac50a74 Mon Sep 17 00:00:00 2001 From: Matheus Buschermoehle Date: Wed, 27 Nov 2024 21:57:02 -0300 Subject: [PATCH 1/4] refactor: extract methods from complex FramedReader.run() method --- .../java/hudson/cli/PlainCLIProtocol.java | 79 +++++++++++-------- 1 file changed, 46 insertions(+), 33 deletions(-) diff --git a/cli/src/main/java/hudson/cli/PlainCLIProtocol.java b/cli/src/main/java/hudson/cli/PlainCLIProtocol.java index e49daa246f88..11a8c7266340 100644 --- a/cli/src/main/java/hudson/cli/PlainCLIProtocol.java +++ b/cli/src/main/java/hudson/cli/PlainCLIProtocol.java @@ -124,46 +124,59 @@ static final class FramedReader extends Thread { public void run() { try { while (true) { - LOGGER.finest("reading frame"); - int framelen; - try { - framelen = dis.readInt(); - } catch (EOFException x) { - side.handleClose(); - break; // TODO verify that we hit EOF immediately, not partway into framelen - } + int framelen = readFrameLength(); if (framelen < 0) { throw new IOException("corrupt stream: negative frame length"); } - LOGGER.finest("read frame length " + framelen); - long start = cis.getByteCount(); - try { - side.handle(new DataInputStream(new BoundedInputStream(dis, /* op byte not counted */framelen + 1))); - } catch (ProtocolException x) { - LOGGER.log(Level.WARNING, null, x); - // but read another frame - } finally { - long actuallyRead = cis.getByteCount() - start; - long unread = framelen + 1 - actuallyRead; - if (unread > 0) { - LOGGER.warning(() -> "Did not read " + unread + " bytes"); - IOUtils.skipFully(dis, unread); - } - } + processFrame(framelen); } - } catch (ClosedChannelException x) { - LOGGER.log(Level.FINE, null, x); - side.handleClose(); - } catch (IOException x) { - LOGGER.log(Level.WARNING, null, flightRecorder.analyzeCrash(x, "broken stream")); - } catch (ReadPendingException x) { - // in case trick in CLIAction does not work - LOGGER.log(Level.FINE, null, x); + } catch (IOException | RuntimeException x) { + handleException(x); + } + } + + private void validateFrameLength(int framelen) throws IOException { + if (framelen < 0) { + throw new IOException("corrupt stream: negative frame length"); + } + } + + private int readFrameLength() throws IOException { + try { + return dis.readInt(); + } catch (EOFException x) { side.handleClose(); - } catch (RuntimeException x) { + throw x; + } + } + + private void processFrame(int framelen) throws IOException { + long start = cis.getByteCount(); + try { + side.handle(new DataInputStream(new BoundedInputStream(dis, framelen + 1))); + } catch (ProtocolException x) { LOGGER.log(Level.WARNING, null, x); - side.handleClose(); + } finally { + skipUnreadBytes(framelen, start); + } + } + + private void skipUnreadBytes(int framelen, long start) throws IOException { + long actuallyRead = cis.getByteCount() - start; + long unread = framelen + 1 - actuallyRead; + if (unread > 0) { + LOGGER.warning(() -> "Did not read " + unread + " bytes"); + IOUtils.skipFully(dis, unread); + } + } + + private void handleException(Exception x) { + if (x instanceof ClosedChannelException || x instanceof ReadPendingException) { + LOGGER.log(Level.FINE, null, x); + } else { + LOGGER.log(Level.WARNING, null, flightRecorder.analyzeCrash((IOException) x, "broken stream")); } + side.handleClose(); } } From 2f7440a49ba7e99b6d4f063eb7fe8069677fb863 Mon Sep 17 00:00:00 2001 From: Matheus Buschermoehle Date: Wed, 27 Nov 2024 22:02:56 -0300 Subject: [PATCH 2/4] refactor: add method that validate framelen non negative --- cli/src/main/java/hudson/cli/PlainCLIProtocol.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cli/src/main/java/hudson/cli/PlainCLIProtocol.java b/cli/src/main/java/hudson/cli/PlainCLIProtocol.java index 11a8c7266340..1737e231206c 100644 --- a/cli/src/main/java/hudson/cli/PlainCLIProtocol.java +++ b/cli/src/main/java/hudson/cli/PlainCLIProtocol.java @@ -125,9 +125,7 @@ public void run() { try { while (true) { int framelen = readFrameLength(); - if (framelen < 0) { - throw new IOException("corrupt stream: negative frame length"); - } + validateFrameLength(framelen); processFrame(framelen); } } catch (IOException | RuntimeException x) { From 080895b1b57d99a4142cdf228662f13f76a1002f Mon Sep 17 00:00:00 2001 From: Matheus Buschermoehle Date: Wed, 27 Nov 2024 22:58:24 -0300 Subject: [PATCH 3/4] refactor: add centralized validation inside Op enum --- cli/src/main/java/hudson/cli/PlainCLIProtocol.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/cli/src/main/java/hudson/cli/PlainCLIProtocol.java b/cli/src/main/java/hudson/cli/PlainCLIProtocol.java index 1737e231206c..d3f453448264 100644 --- a/cli/src/main/java/hudson/cli/PlainCLIProtocol.java +++ b/cli/src/main/java/hudson/cli/PlainCLIProtocol.java @@ -77,6 +77,12 @@ private enum Op { Op(boolean clientSide) { this.clientSide = clientSide; } + + void validate(boolean isClient) throws ProtocolException { + if (this.clientSide != isClient) { + throw new ProtocolException("Operation not allowed on this side: " + this); + } + } } interface Output extends Closeable { @@ -273,7 +279,7 @@ abstract static class ServerSide extends EitherSide { @Override protected final boolean handle(Op op, DataInputStream dis) throws IOException { - assert op.clientSide; + op.validate(false); switch (op) { case ARG: onArg(dis.readUTF()); @@ -332,7 +338,7 @@ abstract static class ClientSide extends EitherSide { @Override protected boolean handle(Op op, DataInputStream dis) throws IOException { - assert !op.clientSide; + op.validate(true); switch (op) { case EXIT: onExit(dis.readInt()); From 6d20b7c38f6a1141c6c1dc6eae2a91741e05d3f8 Mon Sep 17 00:00:00 2001 From: Matheus Buschermoehle Date: Wed, 27 Nov 2024 23:22:52 -0300 Subject: [PATCH 4/4] refactor: replaced conditional with polymorphism applied in handle Op both Client and Server side --- .../java/hudson/cli/PlainCLIProtocol.java | 109 ++++++++++-------- 1 file changed, 63 insertions(+), 46 deletions(-) diff --git a/cli/src/main/java/hudson/cli/PlainCLIProtocol.java b/cli/src/main/java/hudson/cli/PlainCLIProtocol.java index d3f453448264..be173ed35ae3 100644 --- a/cli/src/main/java/hudson/cli/PlainCLIProtocol.java +++ b/cli/src/main/java/hudson/cli/PlainCLIProtocol.java @@ -54,23 +54,69 @@ class PlainCLIProtocol { /** One-byte operation to send to the other side. */ private enum Op { /** UTF-8 command name or argument. */ - ARG(true), + ARG(true) { + @Override + void execute(DataInputStream dis, EitherSide side) throws IOException { + ((ServerSide) side).onArg(dis.readUTF()); + } + }, /** UTF-8 locale identifier. */ - LOCALE(true), + LOCALE(true) { + @Override + void execute(DataInputStream dis, EitherSide side) throws IOException { + ((ServerSide) side).onLocale(dis.readUTF()); + } + }, /** UTF-8 client encoding. */ - ENCODING(true), + ENCODING(true) { + @Override + void execute(DataInputStream dis, EitherSide side) throws IOException { + ((ServerSide) side).onEncoding(dis.readUTF()); + } + }, /** Start running command. */ - START(true), + START(true) { + @Override + void execute(DataInputStream dis, EitherSide side) throws IOException { + ((ServerSide) side).onStart(); + } + }, /** Exit code, as int. */ - EXIT(false), + EXIT(false) { + @Override + void execute(DataInputStream dis, EitherSide side) throws IOException { + ((ClientSide) side).onExit(dis.readInt()); + } + }, /** Chunk of stdin, as int length followed by bytes. */ - STDIN(true), + STDIN(true) { + @Override + void execute(DataInputStream dis, EitherSide side) throws IOException { + ((ServerSide) side).onStdin(dis.readAllBytes()); + } + }, /** EOF on stdin. */ - END_STDIN(true), + END_STDIN(true) { + @Override + void execute(DataInputStream dis, EitherSide side) throws IOException { + ((ServerSide) side).onEndStdin(); + } + }, /** Chunk of stdout. */ - STDOUT(false), + STDOUT(false) { + @Override + void execute(DataInputStream dis, EitherSide side) throws IOException { + ((ClientSide) side).onStdout(dis.readAllBytes()); + } + }, /** Chunk of stderr. */ - STDERR(false); + STDERR(false) { + @Override + void execute(DataInputStream dis, EitherSide side) throws IOException { + ((ClientSide) side).onStderr(dis.readAllBytes()); + } + }; + /** True if sent from the client to the server; false if sent from the server to the client. */ final boolean clientSide; @@ -78,6 +124,8 @@ private enum Op { this.clientSide = clientSide; } + abstract void execute(DataInputStream dis, EitherSide side) throws IOException; + void validate(boolean isClient) throws ProtocolException { if (this.clientSide != isClient) { throw new ProtocolException("Operation not allowed on this side: " + this); @@ -279,29 +327,9 @@ abstract static class ServerSide extends EitherSide { @Override protected final boolean handle(Op op, DataInputStream dis) throws IOException { - op.validate(false); - switch (op) { - case ARG: - onArg(dis.readUTF()); - return true; - case LOCALE: - onLocale(dis.readUTF()); - return true; - case ENCODING: - onEncoding(dis.readUTF()); - return true; - case START: - onStart(); - return true; - case STDIN: - onStdin(dis.readAllBytes()); - return true; - case END_STDIN: - onEndStdin(); - return true; - default: - return false; - } + op.validate(true); + op.execute(dis, this); + return true; } protected abstract void onArg(String text); @@ -338,20 +366,9 @@ abstract static class ClientSide extends EitherSide { @Override protected boolean handle(Op op, DataInputStream dis) throws IOException { - op.validate(true); - switch (op) { - case EXIT: - onExit(dis.readInt()); - return true; - case STDOUT: - onStdout(dis.readAllBytes()); - return true; - case STDERR: - onStderr(dis.readAllBytes()); - return true; - default: - return false; - } + op.validate(false); + op.execute(dis, this); + return true; } protected abstract void onExit(int code);