NationalSecurityAgency / ghidra

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

Class Methods Namespace Not Renaming WIth Parents Classes #4767

Open pinwhell opened 1 year ago

pinwhell commented 1 year ago

Hi, the bug is related to namespaces and this call functions, the bug consist that when Auto Generated Class is renamed, all the Function/Methods that has that namespace ( Class/Struct ) image will get out of scope and wont de-compile, image the function will look for the old ( Namespace/Class ) Name, should not be automatically followed those changes by the main class rename?

To Reproduce this, just find anywhere where you can auto create an struct/class After that, find a function which has the thiscall convention, receiving the previous auto-created Struct object as this pointer Then open the function renamer, and change namespace to this Auto-created Struct, after that, just rename the struct, and function should go out of scope, because still is in the old name of the struct.

The Expected behaviour should be that the this method automaticlly Follow the namespace changes becouse parent Class name also changed

some screenshot after rename image

all comming back to normal after renaming back to old class namespace of the method

image

Environment (please complete the following information):

image

astrelsky commented 1 year ago

Related to #4745, #1210, and whatever others I've missed.

Blatantly obvious problem with an obvious solution, that hasn't been fixed because?

ghidra1 commented 1 year ago

Blatantly obvious problem with an obvious solution, that hasn't been fixed because?

While I will be the first to admit this is severely flawed, a good solution with referential-integrity is not obvious. Underlying issues we are currently faced with include:

Use cases to consider:

@astrelsky I would be curious to hear your recommendations.

In most cases an extensive fuzzy search would be required. If more than one fuzzy-match was found should the rename operation fail? Could be rather painful to identify and diagnoze the fuzzy-associations if this occurs.

Any approach taken initially would be via a utility/Command and not on the low-level API due to the error prone nature.

astrelsky commented 1 year ago

I've been using using the UniversalID to pair a class to the DataType (struct, class or union). This can be painlessly stored in the GhidraClasss' as it's just a long. The documentation for getUniversalID states "This value is intended to be a unique identifier across all programs and archives. The same ID indicates that two datatypes were originally the same one. Keep in mind names, categories, and component makeup may differ and have changed since there origin." If for some reason the DataType with the stored id no longer exists, just create a new one.

I think the UniversalID was the obvious solution I was referring to and have been under the impression that you didn't want to give up those 8 bytes and a database record change. I will admit that I was just following someone else's example "... yell loud enough and someone will eventually come over to see what all the fuss is about... doesn't mean they'll do anything about it...".

Is it GhidraClasses, GhidraClass's or GhidraClass' I have no idea. ¯_(ツ)_/¯

ghidra1 commented 1 year ago

@astrelsky Where do you store this value and how do you get Ghidra to respect the relationship? Assuming Ghidra would respect this relationship, I assume the category and structure name now become meaningless to the association. It would still have the same organizational challenges and how to cope with namespace/category renaming.

astrelsky commented 1 year ago

@astrelsky Where do you store this value and how do you get Ghidra to respect the relationship? Assuming Ghidra would respect this relationship, I assume the category and structure name now become meaningless to the association. It would still have the same organizational challenges and how to cope with namespace/category renaming.

I store it in a database table my plugin uses. I can't actually get Ghidra to respect the relationship without modifying Ghidra itself. All I can do is check when VariableUtilities.findExistingClassStruct et al. return a DataType other then the expected one and log the error.

As far as storing the value in Ghidra it could go right in the SymbolDB's DBRecord used in GhidraClassDB. Obtaining the DataType from the UniversalID is trivial (See DataTypeManager.findDataTypeForID(UniversalID datatypeID)).

There need not be any association of category paths or namespaces. The data layout of a class is independent of it's name, namespace, header file it has been declared in, etc.

ghidra1 commented 1 year ago

@astrelsky, What happens if you rename the structure? Or do you rely on strick renaming convention/technique (i.e., run a script) and/or do you rely on a plugin to listen for symbol rename events which trigger rename of corresponding structure? How do you deal with Ghidra's fuzzy-association for this pointer?

astrelsky commented 1 year ago

@astrelsky, What happens if you rename the structure? Or do you rely on strick renaming convention/technique (i.e., run a script) and/or do you rely on a plugin to listen for symbol rename events which trigger rename of corresponding structure?

The UniversalID stays the same. I never thought about listening for symbol change events. I currently do nothing and keep the same datatype (struct or union) with the old name or vise versa.

How do you deal with Ghidra's fuzzy-association for this pointer?

I log the error when it obtains the wrong data type. There is nothing else I can do.

As for solutions that can be done internally within Ghidra, if a ClassDataType is created to handle classes (struucts and unions) then no sort of event would be necessary to handle it. Renaming a GhidraClass could automatically rename the correct ClassDataType and vise versa without needing to fire off an event to do the work.

ghidra1 commented 1 year ago

As indicated in my initial post, the biggest challenge is overcoming the fuzzy-association and ensuring that it does not break as names are changed. The only good solution requires the use of a fuzzy-association to be eliminated and somehow a 1-to-1 two-way association created and maintained. We would also have to contend with the organization of related datatypes associated with a namespace/class. Establishing a formal organization approach for a namespace/class and its associated datatypes within the datatype manager is where things get very complicated.

astrelsky commented 1 year ago

As indicated in my initial post, the biggest challenge is overcoming the fuzzy-association and ensuring that it does not break as names are changed. The only good solution requires the use of a fuzzy-association to be eliminated and somehow a 1-to-1 two-way association created and maintained. We would also have to contend with the organization of related datatypes associated with a namespace/class. Establishing a formal organization approach for a namespace/class and its associated datatypes within the datatype manager is where things get very complicated.

You overcome the fuzzy-association by removing it as that is the entire problem. Trying to locate a datatype (which has a unique identifier that does not change) by a property the user can freely change at will is foolish.

The means of a one to one association already exists. The UniversalID is a unique id for a datatype that does not change when any of the datatypes other properties change.

Going to throw a curveball here. As far as associating a classes related types in the datatype manager, how about having the class datatype tree node also act like a category path. I've had similar thoughts for organization of templated types.