dod-cyber-crime-center / pyhidra

Pyhidra is a Python library that provides direct access to the Ghidra API within a native CPython interpreter using jpype.
Other
153 stars 14 forks source link

Need ability to fail jvm start on bad vm_args #23

Closed clearbluejar closed 1 year ago

clearbluejar commented 1 year ago

Looking for flexibility to pass custom args to jpype.start. Well, not necessarily all args, but at least ignoreUnrecognized.

https://github.com/dod-cyber-crime-center/pyhidra/blob/718627f8289019cf0433d6a4e9f535f63cd56d3e/pyhidra/launcher.py#L204-L210

When creating my own launch and passing in vm_args, I'd like to be able to set ignoreUnrecognized to False.

This allows me to error out when an arg I pass and expect to work fails...

launcher = pyhidra.HeadlessPyhidraLauncher(verbose)

launcher.add_vmargs(f'-XX:MaxRAMPercentage={max_ram_percent}')
launcher.add_vmargs('-XX:+HeapDumpOnOutOfMemoryError')
launcher.add_vmargs('-XX:+SOME_ERROR_ARG_THAT_MIGHT_FAIL_AND_I_WOULDNT_KNOW')

launcher.start()

Perhaps we could add an optional arg to: https://github.com/dod-cyber-crime-center/pyhidra/blob/718627f8289019cf0433d6a4e9f535f63cd56d3e/pyhidra/launcher.py#L129-L137

def __init__(self, verbose, ignore_unrecognized=True): 

Happy to submit a pull request. Just let me know.

clearbluejar commented 1 year ago

Perhaps there is more to this than I understand.. just saw this: https://github.com/dod-cyber-crime-center/pyhidra/blob/718627f8289019cf0433d6a4e9f535f63cd56d3e/pyhidra/launcher.py#L47-L48

I tested it by setting ignoreUnrecognized changing it, and was able to get the JVM start to fail on an invalid vm_args, but there must be a reason you have the code above.

dc3-tsd commented 1 year ago

Thanks for pointing that out. This option was needed during early development for prior versions of Ghidra, but is no longer used in the versions that Pyhidra supports. A pull request with whatever changes it now needs would be welcome.

clearbluejar commented 1 year ago

Sounds good.

clearbluejar commented 1 year ago

https://github.com/dod-cyber-crime-center/pyhidra/blob/718627f8289019cf0433d6a4e9f535f63cd56d3e/pyhidra/launcher.py#L204-L210

coming back round to this.. do you also need convertStrings=True ?

Seems like that setting forces Java to cast it's strings, but they don't recommend it for new dev. https://github.com/jpype-project/jpype/blob/6869f03fd37184ccfde4db043b2927837e91e072/jpype/_core.py#L147-L152

Does pyhidra / ghidra have known requirements for it, or also a legacy requirement?

clearbluejar commented 1 year ago

answered my question here. looks like convertStrings is required.

image

dc3-tsd commented 1 year ago

This was deemed necessary because converting strings manually would have become heavily tedious with its use in dragodis. However, if the consensus in the community is to not have convertStrings​ on as default, we could just set it only within the dragodis code, now that we have that PR change you provided. Alternatively, we could support not having convertStrings​ on, but keep it on by default to provide less jarring results to the end user.

If we wanted to make pyhidra work with convertStrings​ off, we believe it would just need to go through and manually convert the Java Strings to Python strings in a few places like this.

clearbluejar commented 1 year ago

I am indifferent to the convertStrings issue, I don't know enough about it. I assume there is some overhead to convert all the strings but it is helpful when accessing strings from python? As the pull request has been accepted. I am closing this issue.