From 0fad371a8386ae2dc9e746caf20c61510c8b4aff Mon Sep 17 00:00:00 2001 From: dev747368 <48332326+dev747368@users.noreply.github.com> Date: Tue, 15 Dec 2020 15:14:38 -0500 Subject: [PATCH] GP-487 Fix pasting multi-line string into python console --- .../core/interpreter/InterpreterPanel.java | 191 ++++++------ .../interpreter/InterpreterPanelTest.java | 274 ++++++++++++++++++ .../python/GhidraPythonInterpreter.java | 10 +- .../main/java/ghidra/python/PythonPlugin.java | 7 +- .../python/PythonPluginInputThread.java | 56 ++-- 5 files changed, 400 insertions(+), 138 deletions(-) create mode 100644 Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/interpreter/InterpreterPanelTest.java diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/interpreter/InterpreterPanel.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/interpreter/InterpreterPanel.java index 933189da12..5a4dfc4895 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/interpreter/InterpreterPanel.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/interpreter/InterpreterPanel.java @@ -18,7 +18,10 @@ package ghidra.app.plugin.core.interpreter; import java.awt.*; import java.awt.event.*; import java.io.*; +import java.nio.charset.StandardCharsets; import java.util.List; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.atomic.AtomicBoolean; import javax.swing.*; import javax.swing.text.*; @@ -54,12 +57,12 @@ public class InterpreterPanel extends JPanel implements OptionsChangeListener { private JScrollPane outputScrollPane; private JTextPane outputTextPane; private JTextPane promptTextPane; - private JTextPane inputTextPane; + /* junit */ JTextPane inputTextPane; private CodeCompletionWindow codeCompletionWindow; private HistoryManager history; - private IPStdin stdin; + /* junit */ IPStdin stdin; private OutputStream stdout; private OutputStream stderr; private PrintWriter outWriter; @@ -511,11 +514,10 @@ public class InterpreterPanel extends JPanel implements OptionsChangeListener { private void repositionScrollpane() { // NOTE: CRAZY CODE! subtract one to position short of final newline - outputTextPane.setCaretPosition(outputTextPane.getDocument().getLength() - 1); + outputTextPane.setCaretPosition(Math.max(0, outputTextPane.getDocument().getLength() - 1)); } void addText(String text, TextType type) { - StyledDocument document = outputTextPane.getStyledDocument(); SimpleAttributeSet attributes; switch (type) { case STDERR: @@ -530,6 +532,7 @@ public class InterpreterPanel extends JPanel implements OptionsChangeListener { break; } try { + StyledDocument document = outputTextPane.getStyledDocument(); document.insertString(document.getLength(), text, attributes); repositionScrollpane(); } @@ -562,6 +565,7 @@ public class InterpreterPanel extends JPanel implements OptionsChangeListener { public void clear() { outputTextPane.setText(""); + stdin.resetStream(); } public String getOutputText() { @@ -628,24 +632,7 @@ public class InterpreterPanel extends JPanel implements OptionsChangeListener { public void dispose() { - try { - stdin.close(); - } - catch (IOException e) { - Msg.debug(this, "could not close stdin", e); - } -// try { -// stdout.close(); -// } -// catch (IOException e) { -// Msg.warn(this, "could not close stdout", e); -// } -// try { -// stderr.close(); -// } -// catch (IOException e) { -// Msg.warn(this, "could not close stderr", e); -// } + stdin.close(); setVisible(false); } @@ -674,107 +661,105 @@ public class InterpreterPanel extends JPanel implements OptionsChangeListener { // Inner Classes //================================================================================================== - private class IPStdin extends InputStream { - private byte[] bytes; + + /** + * An {@link InputStream} that has as its source text strings being pushed into + * it by a thread, and being read by another thread. + *

+ * Not thread-safe for multiple readers, but is thread-safe for writers. + *

+ * {@link #close() Closing} this stream (from any thread) will awaken the + * blocked reader thread and give an EOF result to the read operation it was blocking on. + */ + /* junit vis */ static class IPStdin extends InputStream { + private static final byte[] EMPTY_BYTES = new byte[0]; + + // reader-thread only fields. write operations may not access/modify these + // fields. + private byte[] bytes = EMPTY_BYTES; private int position = 0; - private volatile boolean disposed; + // end reader-thread only fields + + // shared reader / writer fields. Any thread may access these as they + // are threadsafe on their own + private LinkedBlockingQueue queuedBytes = new LinkedBlockingQueue<>(); + private AtomicBoolean isClosed = new AtomicBoolean(false); + // end shared fields + + private boolean fetchBytesFromQueue(boolean blocking) { + + try { + // if the current byte buffer is exhausted, loop until we get + // a new non-empty byte buffer. + while (!isClosed.get() && position >= bytes.length) { + byte[] newBytes = blocking ? queuedBytes.take() : queuedBytes.poll(); + if (newBytes == null) { + // this only happens when blocking == false, ie. a poll() operation + break; + } + bytes = newBytes; + position = 0; + } + } + catch (InterruptedException e) { + // fall thru to return which will return false + } + + return position < bytes.length; + } @Override public int read(byte[] b, int off, int len) throws IOException { - while (bytes == null) { - try { - synchronized (this) { - this.wait(); - } - } - catch (InterruptedException e) { - // handled below - } - - if (disposed) { - return -1; - } + if (!fetchBytesFromQueue(true)) { + return -1; } - if (bytes != null) { - int length = Math.min(bytes.length - position, len); - System.arraycopy(bytes, position, b, off, length); - if (position + length == bytes.length) { - position = 0; - bytes = null; - } - else { - position += length; - } - return length; - } - return -1; + int length = Math.min(bytes.length - position, len); + System.arraycopy(bytes, position, b, off, length); + position += length; + return length; } @Override public int read() throws IOException { - while (bytes == null) { - try { - synchronized (this) { - this.wait(); - } - } - catch (InterruptedException e) { - // handled below - } - - if (disposed) { - return -1; - } - + byte[] buffer = new byte[1]; + if (read(buffer, 0, 1) != 1) { + return -1; } - - if (bytes != null) { - int c = bytes[position] & 0xff; - position++; - if (position >= bytes.length) { - position = 0; - bytes = null; - } - return c; - } - return -1; + return buffer[0] & 0xff; } @Override public int available() { - if (bytes == null) { - return 0; + fetchBytesFromQueue(false); + return bytes.length - position; + } + + @Override + public void close() { + // this will wake up a blocked read-thread waiting on a read() operation + // and cause it to return a EOF result. + // All reads() after this close will return EOF value + isClosed.set(true); + queuedBytes.clear(); + queuedBytes.offer(EMPTY_BYTES); + } + + void addText(String text) { + if (!isClosed.get()) { + queuedBytes.offer(text.getBytes(StandardCharsets.UTF_8)); } - return bytes.length; } /** - * Overridden to stop this stream from blocking. - * - * @throws IOException not + * Resets this stream from a closed/always-eof state to an open state. + *

+ * Also clears any queued bytes. Safe to call even when open. */ - @Override - public void close() throws IOException { - disposed = true; - - synchronized (this) { - notify(); // in case we are blocking - } - } - - synchronized void addText(String text) { - if (bytes == null) { - bytes = text.getBytes(); - position = 0; - } - else { - byte[] temp = text.getBytes(); - byte[] newBytes = new byte[bytes.length + temp.length]; - System.arraycopy(bytes, 0, newBytes, 0, bytes.length); - System.arraycopy(temp, 0, newBytes, bytes.length, temp.length); - } - this.notify(); + void resetStream() { + isClosed.set(false); + queuedBytes.clear(); + queuedBytes.offer(EMPTY_BYTES); } } } diff --git a/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/interpreter/InterpreterPanelTest.java b/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/interpreter/InterpreterPanelTest.java new file mode 100644 index 0000000000..9adc528dca --- /dev/null +++ b/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/interpreter/InterpreterPanelTest.java @@ -0,0 +1,274 @@ +/* ### + * IP: GHIDRA + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package ghidra.app.plugin.core.interpreter; + +import static org.junit.Assert.*; + +import java.io.*; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +import javax.swing.*; +import javax.swing.text.BadLocationException; +import javax.swing.text.Document; + +import org.apache.commons.lang3.StringUtils; +import org.junit.*; + +import ghidra.app.plugin.core.console.CodeCompletion; +import ghidra.framework.options.ToolOptions; +import ghidra.test.AbstractGhidraHeadedIntegrationTest; +import ghidra.test.DummyTool; +import resources.Icons; + +/** + * Test the InterpreterPanel/InterpreterConsole's stdIn InputStream handling by + * manually creating a JFrame (while a regular Ghidra tool is running) + * to host the panel in. + */ +public class InterpreterPanelTest extends AbstractGhidraHeadedIntegrationTest { + + private JFrame frame; + private InterpreterPanel ip; + private JTextPane inputTextPane; + private Document inputDoc; + private BufferedReader reader; + + @Before + public void setUp() throws Exception { + + ip = createIP(); + inputTextPane = ip.inputTextPane; + inputDoc = inputTextPane.getDocument(); + reader = new BufferedReader(new InputStreamReader(ip.getStdin())); + frame = new JFrame("InterpreterPanel test frame"); + frame.getContentPane().add(ip); + frame.setSize(400, 400); + runSwing(() -> frame.setVisible(true)); + } + + @After + public void tearDown() throws Exception { + runSwing(() -> { + frame.setVisible(false); + frame.dispose(); + }); + } + + @Test(timeout = 20000) + public void testInputStream_AddRead() throws Exception { + doBackgroundTriggerTextTest(List.of("test1", "abc123")); + } + + @Test(timeout = 20000) + public void testInputStream_ClearResetsStream() throws Exception { + doBackgroundTriggerTextTest(List.of("test1", "abc123")); + doSwingMultilinePasteTest(List.of("testLine1", "testLine2", "testLine3")); + + ip.clear(); + + doBackgroundTriggerTextTest(List.of("test2", "abc456")); + doSwingMultilinePasteTest(List.of("testLine4", "testLine5", "testLine6")); + } + + @Test(timeout = 20000) + public void testInputStream_CloseStreamBeforeReading() throws Exception { + + ip.getStdin().close(); + + assertNull(reader.readLine()); // should always get NULL results because stream is now 'closed' + assertNull(reader.readLine()); // " " + + ip.stdin.addText("test_while_closed\n"); // text added after close shouldn't be preserved + assertNull(reader.readLine()); // should always get NULL results because stream is now 'closed' + ip.clear(); // stream should now be open again + doBackgroundTriggerTextTest(List.of("test2", "abc456")); + } + + @Test(timeout = 20000) + public void testInputStream_CloseStreamWhileBlocking() throws Exception { + + AtomicReference result = new AtomicReference<>("Non-null value"); + doBackgroundReadLine(result); // this thread will block on readLine() + + ip.getStdin().close(); + + // this will be set to null when readLine() returns + waitFor(() -> result.get() == null); + + ip.clear(); // stream should now be open again + doBackgroundTriggerTextTest(List.of("test2", "abc456")); + } + + @Test(timeout = 20000) + public void testInputStream_InterruptStreamWhileBlocking() throws Exception { + + AtomicReference result = new AtomicReference<>("Non-null value"); + Thread t = doBackgroundReadLine(result); // this thread will block on readLine() + + t.interrupt(); + + // this will be set to null when readLine() returns + waitFor(() -> result.get() == null); + + ip.clear(); // stream should now be open again + doBackgroundTriggerTextTest(List.of("test2", "abc456")); + } + + @Test(timeout = 20000) + public void testInputStream_ReadSingleBytes() throws IOException { + + doBackgroundPasteTest1AtATime("testvalue\n"); + + CountDownLatch startLatch = new CountDownLatch(1); + closeStreamViaBackgroundThread(startLatch); + startLatch.countDown(); + + assertEquals(-1, ip.getStdin().read()); + } + + @Test(timeout = 20000) + public void testInputStream_Available() throws IOException { + + assertEquals(0, ip.getStdin().available()); + + triggerText(inputTextPane, "testvalue\n"); + waitForSwing(); + + assertTrue(ip.getStdin().available() > 0); + } + + private InterpreterPanel createIP() { + InterpreterConnection dummyIC = new InterpreterConnection() { + @Override + public String getTitle() { + return "Dummy Title"; + } + + @Override + public ImageIcon getIcon() { + return Icons.STOP_ICON; + } + + @Override + public List getCompletions(String cmd) { + return List.of(); + } + }; + + DummyTool tool = new DummyTool() { + @Override + public ToolOptions getOptions(String categoryName) { + return new ToolOptions("Dummy"); + } + }; + InterpreterPanel result = new InterpreterPanel(tool, dummyIC); + result.setPrompt("PROMPT:"); + return result; + } + + private void doSwingMultilinePasteTest(List multiLineTestValues) + throws IOException { + // simulates what happens during a paste when multi-line string with + // "\n"s is pasted. + runSwingLater(() -> { + try { + String multiLineString = StringUtils.join(multiLineTestValues, "\n") + '\n'; + inputDoc.insertString(0, multiLineString, null); + } + catch (BadLocationException e) { + // ignore + } + }); + + for (String expectedValue : multiLineTestValues) { + String actualValue = reader.readLine(); + assertEquals(expectedValue, actualValue); + } + } + + private void doBackgroundPasteTest1AtATime(String testValue) throws IOException { + runSwingLater(() -> { + try { + inputDoc.insertString(0, testValue, null); + } + catch (BadLocationException e) { + // ignore + } + }); + for (int i = 0; i < testValue.length(); i++) { + char expectedChar = testValue.charAt(i); + int actualValue = ip.getStdin().read(); + assertEquals(expectedChar, (char) actualValue); + } + } + + private void doBackgroundTriggerTextTest(List testValues) throws Exception { + + new Thread(() -> { + for (String s : testValues) { + triggerText(inputTextPane, s + "\n"); + } + }).start(); + + for (String expectedValue : testValues) { + String actualValue = reader.readLine(); + assertEquals(expectedValue, actualValue); + } + } + + private Thread doBackgroundReadLine(AtomicReference result) + throws InterruptedException { + + CountDownLatch startLatch = new CountDownLatch(1); + Thread t = new Thread(() -> { + try { + startLatch.countDown(); + result.set(reader.readLine()); + } + catch (IOException e) { + // test will fail + } + }); + t.start(); + + startLatch.await(2, TimeUnit.SECONDS); // the background thread is now spun-up and ready to read + + // the smallest of sleeps to give the thread a chance to read after the latch was reached + sleep(10); + + return t; + } + + private void closeStreamViaBackgroundThread(CountDownLatch startLatch) { + + new Thread(() -> { + try { + startLatch.await(2, TimeUnit.SECONDS); + + // the smallest of sleeps to give the thread a chance to read after the latch was reached + sleep(10); + + ip.getStdin().close(); + } + catch (InterruptedException | IOException e) { + // ignore + } + }).start(); + } +} diff --git a/Ghidra/Features/Python/src/main/java/ghidra/python/GhidraPythonInterpreter.java b/Ghidra/Features/Python/src/main/java/ghidra/python/GhidraPythonInterpreter.java index 0f338e230c..d9261153f8 100644 --- a/Ghidra/Features/Python/src/main/java/ghidra/python/GhidraPythonInterpreter.java +++ b/Ghidra/Features/Python/src/main/java/ghidra/python/GhidraPythonInterpreter.java @@ -245,8 +245,14 @@ public class GhidraPythonInterpreter extends InteractiveInterpreter { * @param str The string to print. */ void printErr(String str) { - getSystemState().stderr.invoke("write", new PyString(str + "\n")); - getSystemState().stderr.invoke("flush"); + try { + getSystemState().stderr.invoke("write", new PyString(str + "\n")); + getSystemState().stderr.invoke("flush"); + } + catch (PyException e) { + // if the python interp state's stdin/stdout/stderr is messed up, it can throw an error + Msg.error(this, "Failed to write to stderr", e); + } } /** diff --git a/Ghidra/Features/Python/src/main/java/ghidra/python/PythonPlugin.java b/Ghidra/Features/Python/src/main/java/ghidra/python/PythonPlugin.java index f2c24b4e05..667ce8f270 100644 --- a/Ghidra/Features/Python/src/main/java/ghidra/python/PythonPlugin.java +++ b/Ghidra/Features/Python/src/main/java/ghidra/python/PythonPlugin.java @@ -59,6 +59,8 @@ import resources.ResourceManager; public class PythonPlugin extends ProgramPlugin implements InterpreterConnection, OptionsChangeListener { + private final static int INPUT_THREAD_SHUTDOWN_TIMEOUT_MS = 1000; + private InterpreterConsole console; private GhidraPythonInterpreter interpreter; private PythonScript interactiveScript; @@ -202,7 +204,8 @@ public class PythonPlugin extends ProgramPlugin PythonCodeCompletionFactory.setupOptions(this, options); } else { - inputThread.dispose(); + inputThread.shutdown(); + inputThread = null; interpreter.cleanup(); interpreter = GhidraPythonInterpreter.get(); } @@ -278,7 +281,7 @@ public class PythonPlugin extends ProgramPlugin // Terminate the input thread if (inputThread != null) { - inputThread.dispose(); + inputThread.shutdown(); inputThread = null; } diff --git a/Ghidra/Features/Python/src/main/java/ghidra/python/PythonPluginInputThread.java b/Ghidra/Features/Python/src/main/java/ghidra/python/PythonPluginInputThread.java index 90c7dee1e7..3057e2aae9 100644 --- a/Ghidra/Features/Python/src/main/java/ghidra/python/PythonPluginInputThread.java +++ b/Ghidra/Features/Python/src/main/java/ghidra/python/PythonPluginInputThread.java @@ -28,10 +28,11 @@ class PythonPluginInputThread extends Thread { private static int generationCount = 0; - private PythonPlugin plugin; + private final PythonPlugin plugin; + private final AtomicBoolean moreInputWanted = new AtomicBoolean(false); + private final AtomicBoolean shutdownRequested = new AtomicBoolean(false); + private final InputStream consoleStdin; private PythonPluginExecutionThread pythonExecutionThread; - private AtomicBoolean moreInputWanted; - private AtomicBoolean shouldContinue; /** * Creates a new python input thread that gets a line of python input from the given plugin. @@ -41,8 +42,7 @@ class PythonPluginInputThread extends Thread { PythonPluginInputThread(PythonPlugin plugin) { super("Python plugin input thread (generation " + ++generationCount + ")"); this.plugin = plugin; - this.moreInputWanted = new AtomicBoolean(false); - this.shouldContinue = new AtomicBoolean(true); + this.consoleStdin = plugin.getConsole().getStdin(); } /** @@ -56,28 +56,13 @@ class PythonPluginInputThread extends Thread { @Override public void run() { - try (BufferedReader reader = - new BufferedReader(new InputStreamReader(plugin.getConsole().getStdin()))) { - while (shouldContinue.get()) { - - // Read a line from the console. Do it non-blocking so we can exit the thread - // if we were instructed to not continue. - String line; - if (plugin.getConsole().getStdin().available() > 0) { - line = reader.readLine(); - } - else { - try { - Thread.sleep(50); - } - catch (InterruptedException e) { - // Nothing to do...just continue. - } - continue; - } + try (BufferedReader reader = new BufferedReader(new InputStreamReader(consoleStdin))) { + String line; + while (!shutdownRequested.get() && (line = reader.readLine()) != null) { // Execute the line in a new thread - pythonExecutionThread = new PythonPluginExecutionThread(plugin, line, moreInputWanted); + pythonExecutionThread = + new PythonPluginExecutionThread(plugin, line, moreInputWanted); pythonExecutionThread.start(); try { @@ -90,9 +75,10 @@ class PythonPluginInputThread extends Thread { } // Set the prompt appropriately - plugin.getConsole().setPrompt( - moreInputWanted.get() ? plugin.getInterpreter().getSecondaryPrompt() - : plugin.getInterpreter().getPrimaryPrompt()); + plugin.getConsole() + .setPrompt( + moreInputWanted.get() ? plugin.getInterpreter().getSecondaryPrompt() + : plugin.getInterpreter().getPrimaryPrompt()); } } catch (IOException e) { @@ -103,9 +89,17 @@ class PythonPluginInputThread extends Thread { } /** - * Disposes of the thread by telling it to not continue asking for input. + * Causes the the background thread's run() loop to exit. + *

+ * Causes background thread's exit by closing the inputstream it is looping on. */ - void dispose() { - shouldContinue.set(false); + void shutdown() { + try { + shutdownRequested.set(true); + consoleStdin.close(); + } + catch (IOException e) { + // shouldn't happen, ignore + } } }