search

NationalSecurityAgency / ghidra

Ghidra is a software reverse engineering (SRE) framework
https://www.nsa.gov/ghidra
Apache License 2.0
50.9k stars 5.8k forks source link

Unchecked exception NoTransactionException #1838

Open Learath2 opened 4 years ago

Learath2 commented 4 years ago

Should this maybe be a checked exception? It causes some very subtle issues in scripts that can get pretty tough to diagnose.

Like if you use GhidraScript.setBackgroundColor() in the execute() method of a TableChooserExecutor it will silently fail.

astrelsky commented 4 years ago

Should this maybe be a checked exception? It causes some very subtle issues in scripts that can get pretty tough to diagnose.

Like if you use GhidraScript.setBackgroundColor() in the execute() method of a TableChooserExecutor it will silently fail.

If NoTransactionException were to be made a checked exception then this would need to be checked for everywhere. Every method that modifies a program, datatype, datatype manager, symbol, listing, namespace, function, etc would need to have the throws clause added for NoTransactionException. It would be a maintenance nightmare for both the developers and for user extensions/scripts.

When changes to a database are made a transaction must be started, the changes made, and then the transaction is ended. This exception occurs when modifications are attempted to a database prior to starting a transaction. If this occurs it isn't something that can be recovered from and is the result of a programming error.

With that being said this shouldn't be something that occurs through the scripting api. May you please submit an example to reproduce this.

Learath2 commented 4 years ago

Hm, I agree that it'd be a hassle, if we can't make these checked, could we maybe atleast log these?

This reproduces the problem, it happens because when the script ends the transaction created by FlatProgramAPI.start() ends in FlatProgramAPI.end() however the dialog outlasts the script itself. Currently it silently fails, leaving a bug that is not very pleasent to diagnose.

astrelsky commented 4 years ago

Hm, I agree that it'd be a hassle, if we can't make these checked, could we maybe atleast log these?

This reproduces the problem, it happens because when the script ends the transaction created by FlatProgramAPI.start() ends in FlatProgramAPI.end() however the dialog outlasts the script itself. Currently it silently fails, leaving a bug that is not very pleasent to diagnose.

So I was able to reproduce this. This is certainly an issue with GhidraScript. Either the TableChooserDialog returned by GhidraScript.createTableChooserDialog should be a wrapper around the instance returned by the table service which prevents the script from via a CancellableFunction, or GhidraScript needs to ensure that a transaction has been started in setBackgroundColor.

Learath2 commented 4 years ago

I don't think we should drag on the transaction from the script, a checkTransaction inside any GhidraScript function that needs a transaction feels like the better idea.