Closed TheZ3ro closed 5 months ago
Thank you for catching this. Indeed the environment variable should be checked before any function uses it, so moving it up the constructor makes sense.
Incidentally, we are work on a slight refactoring such that the ghidra install directory can be configured during construction of the launcher, with GHIDRA_INSTALL_DIR set as the default if not provided. This should make the Launcher class the source of truth for configuration of the Ghidra launch so it will eventually become more like:
def __init__(self, install_dir = None):
self.install_dir = install_dir or os.getenv("GHIDRA_INSTALL_DIR")
This PR change the place where the
GHIDRA_INSTALL_DIR
env variable is checked inPyhidraLauncher
, checking it at the top ofPyhidraLauncher
's constructor. Currently the variable is checked inside thestart
function.Rationale When in a "bad" state, the program should exit as soon as possible to avoid continuing the execution.
The fact that the environment variable is checked later in the
start
function allows some weird behavior, specifically the following python "pseudocode" will crash:This is because inside the
__init__
constructor,self.vm_args
is set to the return value of_jvm_args()
, in pyhidra/launcher.py#L134.The
_jvm_args
function will returnNone
since theGHIDRA_INSTALL_DIR
is empty/not set, in pyhidra/launcher.py#L40.When
self.add_vmargs
is called later, it will trigger aTypeError: unsupported operand type(s) for +=: 'NoneType' and 'tuple'
, in pyhidra/launcher.py#L148.For example, ghidriff by @clearbluejar is affected ghidriff/ghidra_diff_engine.py#L98-L120