Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrates to use JepConfig stdout, stderr from setStream method #36

Merged
merged 7 commits into from
Apr 17, 2023

Conversation

ooprathamm
Copy link
Contributor

Fixes #25

@ooprathamm
Copy link
Contributor Author

ooprathamm commented Mar 27, 2023

I have made an attempt to remove the setStreams method and using JepConfig's std out, err .
The JepConfig methods excpect Outputstream objects but ghidra.app.plugin.core.interpreter.InterpreterConsole seems to return PrintWriters so I have used apache WriterOutputStream for this purpose

We can also look to adding a wrapper around PrintWriter objects rather than using WriterOutputStream

Copy link
Collaborator

@mike-hunhoff mike-hunhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ooprathamm! Looks good so far - I left a couple of comments to address.

Comment on lines 73 to 74
config.redirectStdout(new WriterOutputStream(out, "UTF-8"));
config.redirectStderr(new WriterOutputStream(err, "UTF-8"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we foresee any potential encoding issues by manually specifying UTF-8? e.g. Windows v Linux v Mac?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

As per the apache docs Charset is a required param and specifiying it explicitly does look out for possible encoding issues

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood! Is there any way for us to determine the encoding at runtime? e.g. is there a method we can call on out or err to get the encoding used and pass it to WriterOutputStream?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the method from java.nio.Charsets that provides us with the encoding used by the jvm (which should be the encoding used )
image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I think this looks good. Can you please include a screenshot of the unit tests passing in your environment w/ these changes? You can run this script from within Ghidra to kick off the tests.

@ooprathamm
Copy link
Contributor Author

*Passing tests
image
I expected java.nio to be preinstalled, so rather than having it as dependency i have migrated to use System.getProperty() to get the encoding.

@mike-hunhoff
Copy link
Collaborator

*Passing tests image I expected java.nio to be preinstalled, so rather than having it as dependency i have migrated to use System.getProperty() to get the encoding.

Can you run our example script in your environment? I'm not seeing print output in the Ghidrathon or Ghidra console windows.

@ooprathamm
Copy link
Contributor Author

ooprathamm commented Mar 31, 2023

*Passing tests image I expected java.nio to be preinstalled, so rather than having it as dependency i have migrated to use System.getProperty() to get the encoding.

Can you run our example script in your environment? I'm not seeing print output in the Ghidrathon or Ghidra console windows.

*Print Output of console window
image

@ooprathamm
Copy link
Contributor Author

There seems to be something wrong with Ghidrathon. I will look into it.

@ooprathamm
Copy link
Contributor Author

@mike-hunhoff Pardon for the late response, I got occupied by exams.
I was unable to triage the cause of broken interpreter window, any guidance could be useful.

@mike-hunhoff
Copy link
Collaborator

@mike-hunhoff Pardon for the late response, I got occupied by exams. I was unable to triage the cause of broken interpreter window, any guidance could be useful.

Were you able to isolate that changes introduced in this branch are the cause? Or do you believe something else may be causing the issue?

@ooprathamm
Copy link
Contributor Author

ooprathamm commented Apr 6, 2023

@mike-hunhoff Pardon for the late response, I got occupied by exams. I was unable to triage the cause of broken interpreter window, any guidance could be useful.

Were you able to isolate that changes introduced in this branch are the cause? Or do you believe something else may be causing the issue?

yes, the changes are the cause.
I suspect that redirecting streams using jep is causing an overlapping(maybe) for the output stream for the script and the thread (because errors are printed but welcome and outputs aren't)

@ooprathamm
Copy link
Contributor Author

@mike-hunhoff the broken interpreter window has been fixed.

  • Run tests

image

*Run ghidrathon_example.py
image

*Interpreter Window
image

Copy link
Collaborator

@mike-hunhoff mike-hunhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great @ooprathamm , thank you!

@mike-hunhoff mike-hunhoff merged commit 8142d7a into mandiant:main Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove stream workaround
2 participants