RTTI Analysis

[astrelsky]

@ghidra007 I figured I'd finally open the discussion. There is a lot that can be discussed here. I'm not the best at creating things of this sort so I'm just gonna wing it with not much thought put into it.

Some gotchas during analysis:

• Data Relocations
• Windows Control Flow Guard
• 64 bit PowerPC Function Descriptors
• Android Vftables

The data relocations I believe have been taken care of.

Windows control flow guard is something I never figured out how to deal with. It gets in the way because it appears that the compiler will insert pointers to the call checking functions (I forget the name) into the vftable.

64 bit PowerPC's function descriptors are a part of the abi. All you really need to know about this wrt rtti is that all function pointers really point to a structure whose first member is the actual function pointer. A that is needed here is an additional "dereference". It seems to apply to 64-32addr as well which I recently learned.

Android vftables don't seem to "follow the rules" wrt virtual destructor placement. It is not safe to assume that the first two entries are destructors. Iirc they are still present in the table but don't quote me on that.

Has any thought been put into proper handling of virtual inheritance in both the java end software modeling and in the decompiler?

Have #1431 and #1210 been seen as a bigger problems yet?

[ghidra007]

Thanks for opening the discussion. I don't know the answers to all your questions.

I assume you mean the data relocation issue when referencing into the EXTERNAL block? There was a change made to help with this issue in 10.0 although it isn't a complete fix yet.

Thanks for the heads up about the other issues with the RTTI analyzer. I'll add those issues to the list.

Both issues you mentioned are known big issues that many would like to see solved.

If you can point us to sample programs that have the above issues you mentioned, we can use them to triage and test when we get to those issues. Thanks.

[astrelsky]

I was referencing the relocations in the EXTERNAL block. I don't think it truly can be fixed unless the data from the external program is available when during relocation handling.

The mentioning of the issues was purely out of curiosity as I see the addition of more rtti and c++ analysis as opening the doors to more c++ reversing support. #1431 has an attached sample and the problem with #1210 is that the searcher expects there to only be one class with said name. If there are multiple classes with the same name and the types are in different categories it can retrieve the wrong one and send it to the decompiler.

Just for clarification the rtti analysis things mentioned were just things to look ouut for and not necessarily existing issues.

[ghidra007]

Thanks for the clarifications. The structure issue definitely needs a solution.

[astrelsky]

I just ran the recover classes script on my inheritance tests built with visual studio. I noticed that it has put the vbtablePtr in the wrong position. I never managed to properly figure out the ordering for where the vbtable/vftable pointers go when both are present. It never appeared to follow a set rule and, like most things the made by ms do, the compiler just seems to do whatever it wants and place them in whatever order it feels like. I don't remember if I ever released a compiled version of the test classes with visual studio so I'll drop it here.

InheritanceTests.zip
The assignment can be seen in the function starting at 0x1400c9d90.

Also there is an undocumented, at least I never found it documented, bit in the BaseClassDescriptor attributes field if it is a virtual base class. I'm to tired to calculate the bit position so instead I'll just show the calculation. The model here is an instance of Rtti1Model (model.getAttributes() >> 4 & 1) == 1.

[ghidra007]

I'll have to dig into how I figured them out. It was complicated to figure out and i can't remember what docs i used. i'll try to dig them up. I used info from the rtti structs and also from how it is used in the decompiler. It was complicated and i didn't have a lot of examples with answers so i'm sure it could use more testing/work. Thanks for the examples.

[astrelsky]

No problem. This is something that is much easier with the gcc and Itanium ABI layout since there is only one pointer.

ghidra/Ghidra/Features/Decompiler/ghidra_scripts/classrecovery/RTTIGccClassRecoverer.java

Lines 1508 to 1512 in 020df70

	// from doc:
	//All but the lower 8 bits of __offset_flags are a signed offset. For a
	//non-virtual base, this is the offset in the object of the base subobject.
	//For a virtual base, this is the offset in the virtual table of the
	//virtual base offset for the virtual base referenced (negative).

You should try to do it this way, however I believe I encountered problems when trying that method. What I eventually ended up doing is represented by the below pseudocode:

interface TypeInfo {

    // returns an **insertion order** set of the virtual parents
    Set<TypeInfo> getVirtualParents();
}

Set<TypeInfo> getVirtualParents() {
    Set<ClassTypeInfo> result = new LinkedHashSet<>();
        for (BaseClassTypeInfoModel base : bases) {
            ClassTypeInfo parent = base.getClassModel();
            if (base.isVirtual()) {
                result.add(parent);
            }
            result.addAll(parent.getVirtualParents());
        }
    return result;
}

Once you have that ordered set you can just iterate and traverse the vtable from bottom to top (after offset value 0).
If you think a flowchart or something would help more than pseudocode let me know and I can make one. It would likely be hand drawn though.

[ghidra007]

Thanks for the suggestion. That looks like a much cleaner solution. I've been doing a lot of work in this section of code so it looks different now. I now make use of the flags to set isPublic and isVirtual for the classes. I've also improved finding and defining vtables in stripped binaries and finding VTT's and construction-vtables. I'm slowly working towards getting gcc more fully functional. I have been mostly concentrating on getting things to work then will go back through and tighten things up. I'm not sure when it will get pushed out but am hoping the gcc will be more usable then.

[astrelsky]

fysa there are multiple IP and license headers in the Ghidra/Features/Decompiler/ghidra_scripts/classrecovery files.

[ghidra007]

Thanks. I'll take a look.

[astrelsky]

Regarding #3266 and so that this is readily findable in the future, it is possible for both the "in charge" and "not in charge" virtual destructor slots to be null. When this occurs it is usually and abstract base and in my experience it most commonly occurs in construction vtables.

I think this also happens if you explicitly declare a non virtual destructor on a class that has virtual functions but don't quote me on it.

[ghidra007]

Thanks. Good to know. I just finished code that figures out the starts of vtables (main, internal, and construction) and the VTTs for stripped gccs and am doing a better job defining them in both cases.

[Wall-AF]

Will any of this be applicable to Borland C++ (v 4.0, 4.02, 4.5, etc.) / Microsoft (mfc 1.0, 1.1, 1.51, etc) of the mid-1990's???

[ghidra007]

Is this a general Ghidra issue or related to RTTI for Borland?

[Wall-AF]

My current focus is on REing a Windows 3.1X application suit, but I think its safe to assume others would also experience these awkward issues with segmented architectures.

[ghidra007]

The reason I asked is because the issue will probably be lost in this thread. It should probably be added as its own issue since it is separate from RTTI. That will make it more likely to be seen by people with more knowledge of segmented architectures.

[Wall-AF]

I have this one #2315 outstanding for a while.

[ghidra007]

Ok. Thanks.

[0xBEEEF]

I have a question about additive VFTables. Up to now, a separate structure, including VFTable, is created for each class. In the inherited classes, in turn, the underlying class is always referenced, in that the data type of the underlying structure is directly included in the current one. But now I have a problem with additive VFTables. Imagine the following example:

class A {
   public:
    virtual int MethodA() = 0;
};

class B : public A {
   public:
    virtual int MethodA() = 0;
    virtual int MethodB() = 0;
};

class C : public B {
   public:
    int MethodA() { return 5; }
    int MethodB() { return 10; }
    virtual int MethodC() = 0;
};

class D : public C {
   public:
    int MethodA() { return 5; }
    int MethodB() { return 10; }
    int MethodC() { return 15; }
};

The following data types are created (simplified example):

struct A
{
  struct A_VFTable;
  struct A_Data;
}
struct B
{
  struct A;
  struct B_Data;
}
struct C
{
  struct B;
  struct C_Data;
}
struct D
{
  struct C;
  struct D_Data;
}

During compilation the following VFTables are created. On some levels further virtual methods are added in it.

const D::`vftable' DQ   FLAT:const D::`RTTI Complete Object Locator'                  ; D::`vftable'
        DQ      FLAT:virtual int D::MethodA(void)
        DQ      FLAT:virtual int D::MethodB(void)
        DQ      FLAT:virtual int D::MethodC(void)
const C::`vftable' DQ   FLAT:const C::`RTTI Complete Object Locator'                  ; C::`vftable'
        DQ      FLAT:virtual int C::MethodA(void)
        DQ      FLAT:virtual int C::MethodB(void)
        DQ      FLAT:_purecall
const B::`vftable' DQ   FLAT:const B::`RTTI Complete Object Locator'                  ; B::`vftable'
        DQ      FLAT:_purecall
        DQ      FLAT:_purecall
const A::`vftable' DQ   FLAT:const A::`RTTI Complete Object Locator'                  ; A::`vftable'
        DQ      FLAT:_purecall

Because now in Ghidra with the help of the RTTI script only per level a VFTable is generated, and this is used in the next higher class likewise, there are naturally problems with the resolution of additive virtual methods. In the code these are not found accordingly, because naturally the enclosing classical with the additional additive methods only those of the internal know.

Example:
C knows only the method of A, because the structure of A is directly included, and is passed through all hierarchies. A call of C->MethodB() is not recognized therefore but again only with confused mathematical offset computations supplemented. The same applies also to D->MethodC().

I address the question here directly to @ghidra007 and @astrelsky. How do you deal with something like this? Unfortunately, I can't help myself any other way than either to install the large VFTables with all virtual methods in the lower classes, or not to include the VFTables below as a direct reference in the structure. Is there a solution for this?

[ghidra007]

Thanks for the info and for looking for tests.

[ghidra007]

By creating a reference on the listing's call instruction operand and setting the reference type to CALL_OVERRIDE_UNCONDITIONAL, I was able to manually make the decompiler show the correct function called at least in the case where there is an easy way to ID the corresponding call instruction in the listing. I'm going to play around with this a bit programmatically to see if I can generalize it or see if the decompiler guru can do it with the decompiler internals instead. I'm not sure if it is the best way but it looks like a possible solution for now at least in some cases. I have no idea how easy it will be to programmatically id the correct locations to put the reference on. I think coming at it from the decompiler side might be more doable but again, I'm only speculating. With the method I just listed, the decompiler not only shows cleaner and correct output but now it is also navigable to the correct vfunction when clicking on the function call from the decompiler.

[astrelsky]

What about instead being able to override the value of a structure field. This would allow the value of the vptr to be assigned to a "constant" value so that the data at said location could be used. This would be helpful for displaying more correct results for virtual inheritance as well. In order for it to work the decompiler would need to discard the pointer type and instead directly use the constand value and type from the loaded address. Having a special pointer type that would always float and be intended only for static global data could work miraculously.

From there an analyzer could go through and manually set these overrides to the correct location for the most derived (or least I don't remember how that works) base class. (Always the oldest parent class unless otherwise known). The user could then change them as needed if they needed to be different. These pointer values could later be associated with the GhidraClass so that they don't need to be done by analysis iff it works out.

[ghidra007]

That's an interesting idea. I'll discuss it with the team to see what they think of it.

[ghidra007]

Also, be careful when using the manual technique I described above. It is good for the case where you definitively know which vftable to use (ie you see the constructor called from the same method and know which vftable was assigned in it, or there are no derived classes) but if you can't determine this, there could be multiple possibilities and it should be left as is.

[astrelsky]

@ghidra007 what are you looking for in regards to tests?

[ghidra007]

I thought I wanted the aforementioned example but am all good now, thanks. I was able to find something similar in my test files.

[ghidra007]

The upcoming 10.1 has some improvements/changes that should help with many of the issues described above. The class structures have been changed so that the base structures in a derived class structure are split into parts instead of being lumped in as a whole. This makes it more difficult to understand them when looking at the structures themselves but fixes the issues where the wrong vftable and vbtable pointers were used. The virtual function signatures have also been improved so that they are no longer assigned a specific class this* which allows the signatures to be assigned to the correct class now.

[jrmuizel]

Are those changes already in master?

[astrelsky]

Are those changes already in master?

I don't think so. I could be wrong though.

[ghidra007]

They were pushed into our dev master yesterday. A lot of things have been as we are almost ready to freeze 10.1. I believe once the overnight test breakages get resolved master will get pushed out. The person who normally does this is out so I'm not sure when it will happen. I'll comment here once I know for sure.

[ghidra007]

I believe so. I'd appreciate you trying it on your example binary to make sure.

[ryanmkurtz]

I believe once the overnight test breakages get resolved master will get pushed out.

It's been pushed to GitHub now.

[0xBEEEF]

One more question about the planned 10.1 release. Today the changelog or what's new part was already published. Unfortunately I didn't read anything about the "Shifted Pointers" or "Pointers with Offset". But the commit is still in the master. Is this finally included in the 10.1 version, or are only the "beginnings" included in the decompiler? Please for short clarification.

Maybe @emteere can say something about this, because the changelog here was created/maintained by him.

Thanks a lot!

[emteere]

10.1 will include some refactoring in the decompiler done to support pointers with an offset and other types of pointers, such as pointers to a particular address space. It made sense to work them in during data type recovery changes in the decompiler. The rest of the required refactoring for the feature wasn't ready for the 10.1 release. I can't promise a particular time frame for a release, however the feature is in active development and one of the highest priorities. I anticipate it being in the next release following 10.1 baring any complications. Some changes to the database and working it in consistently to the data type models have been required.

So you didn't miss anything in the beta release, thanks for the interest. We know it is high on everyone's wish list.

[0xBEEEF]

10.1 will include some refactoring in the decompiler done to support pointers with an offset and other types of pointers, such as pointers to a particular address space. It made sense to work them in during data type recovery changes in the decompiler. The rest of the required refactoring for the feature wasn't ready for the 10.1 release. I can't promise a particular time frame for a release, however the feature is in active development and one of the highest priorities. I anticipate it being in the next release following 10.1 baring any complications. Some changes to the database and working it in consistently to the data type models have been required.

So you didn't miss anything in the beta release, thanks for the interest. We know it is high on everyone's wish list.

Is there any news on this point? I currently fail mainly on this missing feature in the analysis, because there are very many relative pointers by compiler optimizations. So I just want to say again that I feel this is really important, and I would be happy if it was somehow included in the next release. I would be very happy about a short statement about this.

[astrelsky]

This check here is a bit faulty considering that "default" is equivalent to "gcc" for a large majority of language definitions.

ghidra/Ghidra/Features/Decompiler/ghidra_scripts/RecoverClassesFromRTTIScript.java

Lines 539 to 541 in 0e3da48

	boolean isCompilerSpecGcc =
	currentProgram.getCompilerSpec().getCompilerSpecID().getIdAsString().equalsIgnoreCase(
	"gcc");

[ghidra007]

Thanks for the info!

[ghidra007]

The isGcc method isn't just relying on that check. If it is true great. If not, the code below that check, is handling the cases where it isn't returning true and checking the comment section to see if it is gcc.

[0xBEEEF]

@ghidra007
I have another example here that I'm a little unsure about. It contains RTTI data, also references to VFTables. But both the auto-analysis and your recovery script don't detect anything in this example.

There are some RTTI structures, which also have a name for classes, and there are even references to VFTables in them. But this is not recognized by the whole system. Could you please check what is wrong with this, or if it is not possible to create at least class structures from the existing RTTI data and use the existing VFTable information?

If it is a bug from your point of view, I can always open an issue about it. Just wanted to clarify in advance if you already had to deal with such examples, or could make an extension to the routines, so that such truncated RTTI data can also be handled.

Thanks for your support in advance.
Test.zip.txt

[emteere]

Looks like an older version of RTTI, where the RTTIBaseClassDescriptor does not have a pointer to a RTTIClassHierarchyDescriptor as the last element in the structure.

If you could please create a new ticket for it, we'll see if it can make it into the next release or patch release. I've already got a temporary fix but it will need more testing to make sure it doesn't break newer versions.

[Phylante]

You nailed it.
For reference, the full "what is happening" stuff is here: #1790
If your fix is not already included in the latest release, can you provide a branch or patch for me to test ?

[ghidra007]

I'll try to take a look and see what is going on. Thanks!

[0xBEEEF]

@ghidra007

Since the new version and the adjustments, I have encountered a llegalArgumentException in a few cases at the following location. The error always occurred when the proportions did not match, i.e. in this case there was not enough space in the structure.

ghidra/Ghidra/Features/Decompiler/ghidra_scripts/classrecovery/EditStructureUtils.java

Lines 257 to 259 in 56a8346

	else {
	structure.replaceAtOffset(offset, dataType, -1, fieldName, null);
	}

And here, too, the script has already blown up in my face. Here it is not checked whether it is a valid symbol name. There are template classes that have characters in their names that cause the programme to abort at this point. I think you should therefore check all names beforehand and clean them up if necessary so that the script runs over them without errors.

ghidra/Ghidra/Features/Decompiler/ghidra_scripts/classrecovery/RTTIWindowsClassRecoverer.java

Lines 2680 to 2682 in 56a8346

	primarySymbol.setName("vftable_for_" + baseClassName,
	primarySymbol.getSource());
	}

Unfortunately, I can't provide you with examples directly, I'm sorry.

[ghidra007]

Thank you this helped identify the issue.

[ghidra007]

Can you replace the method with the following and verify that it fixes your issue. Thanks!

static Structure addDataTypeToStructure(Structure structure, int offset,
DataType dataType, String fieldName, TaskMonitor monitor)
throws CancelledException {

	if (structure.isPackingEnabled()) {
		throw new IllegalArgumentException(
			"Packed structures are not supported by this method");
	}

	if (structure.isZeroLength() || offset >= structure.getLength()) {
		try {
			structure.insertAtOffset(offset, dataType, -1, fieldName, null);
		}
		catch (IllegalArgumentException e) {
			DataTypeComponent component = structure.getComponentContaining(offset);
			String error = "\nError inserting at offset " + offset + ". \nStructure: \n" +
				structure.toString() + "\nComponent being inserted:\n" +
				component.getDataType().toString() + " component offset " +
				component.getOffset() + " component len = " +
				component.getDataType().getLength() + "\nInserting DataType: \n" +
				dataType.toString() + " length = " + dataType.getLength();
			throw new IllegalArgumentException(error);
		}
	}
	else {
		try {
			// if not enough room, grow the structure
			int roomForData = structure.getLength() - (offset + dataType.getLength());
			if (roomForData < 0) {
				structure.growStructure(0 - roomForData);
			}
			structure.replaceAtOffset(offset, dataType, -1, fieldName, null);
		}
		catch (IllegalArgumentException e) {
			DataTypeComponent component = structure.getComponentContaining(offset);
			structure.replaceAtOffset(offset, dataType, -1, fieldName, null);
			String error = "\nError replacing at offset " + offset + ". \nStructure: \n" +
				structure.toString() + "\nComponent being replaced:\n" +
				component.getDataType().toString() + " component offset " +
				component.getOffset() + " component len = " +
				component.getDataType().getLength() + "\nReplacement DataType: \n" +
				dataType.toString() + " length = " + dataType.getLength();
			throw new IllegalArgumentException(error);
		}

	}
	return structure;
}

[0xBEEEF]

Thanks for the quick correction. But unfortunately you still have a mistake in the replace part, namely in the catch block you accidentally have the function call of structure.replaceAtOffset inside again. This leads to the fact that your new exception is not thrown, but only the original one. Please remove this line.

Thanks for the adjustments. I will test it again next week and come back to you in case of further problems.

[0xBEEEF]

Was able to successfully test your correction today. Thus, no more errors occur in my specific case.

[ghidra007]

Sorry about pasting the extra line there. Thanks for testing.

[astrelsky]

@ghidra007 it appears that RTTIGccClassRecoverer::createTypeinfoStructs is considering references to the __vmi_class_typeinfo _vptr in executable memory blocks. Specifically .text. I encountered this while following back an error caused by trying to use a negative number of bases.

[astrelsky]

I don't think so it fails elsewhere. Here is the elsewhere:

stacktrace

Error running script: RecoverClassesFromRTTIScript.java
java.lang.reflect.InvocationTargetException
	at ghidra.app.plugin.core.analysis.AutoAnalysisManager$AnalysisWorkerCommand.applyTo(AutoAnalysisManager.java:1713)
	at ghidra.app.plugin.core.analysis.AutoAnalysisManager$AnalysisTaskWrapper.run(AutoAnalysisManager.java:688)
	at ghidra.app.plugin.core.analysis.AutoAnalysisManager.startAnalysis(AutoAnalysisManager.java:788)
	at ghidra.app.plugin.core.analysis.AutoAnalysisManager.startAnalysis(AutoAnalysisManager.java:667)
	at ghidra.app.plugin.core.analysis.AutoAnalysisManager.startAnalysis(AutoAnalysisManager.java:632)
	at ghidra.app.plugin.core.analysis.AnalysisBackgroundCommand.applyTo(AnalysisBackgroundCommand.java:58)
	at ghidra.framework.plugintool.mgr.BackgroundCommandTask.run(BackgroundCommandTask.java:102)
	at ghidra.framework.plugintool.mgr.ToolTaskManager.run(ToolTaskManager.java:336)
	at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: java.lang.Exception: money_get_shim<char> trying to add different vftable address (old: 08207400 new: 08207318)  to same offset 0
	at classrecovery.RecoveredClass.addClassOffsetToVftableMapping(RecoveredClass.java:210)
	at classrecovery.RTTIGccClassRecoverer.processGccRTTI(RTTIGccClassRecoverer.java:270)
	at classrecovery.RTTIGccClassRecoverer.createRecoveredClasses(RTTIGccClassRecoverer.java:106)
	at RecoverClassesFromRTTIScript.run(RecoverClassesFromRTTIScript.java:274)
	at ghidra.app.script.GhidraScript.executeNormal(GhidraScript.java:379)
	at ghidra.app.script.GhidraScript$1.analysisWorkerCallback(GhidraScript.java:361)
	at ghidra.app.plugin.core.analysis.AutoAnalysisManager$AnalysisWorkerCommand.applyTo(AutoAnalysisManager.java:1707)
	... 8 more

I'll see about throwing together a quick sample for the issue I encountered earlier. References to it in code is normal though as it is itself a class.

[ghidra007]

Thanks for the stack trace. It gives me a much better idea about what is going on.

[astrelsky]

Thanks for the stack trace. It gives me a much better idea about what is going on.

I don't think they are the same issue though.

[ghidra007]

Thanks for the clarification. I thought you were implying they were related originally but it sounds like you mean they are two separate issues. The gcc class recovery needs a lot of work. It is extremely prototype at this point. Appreciate the help identifying issues.

[ghidra007]

For the stack trace case, the code thinks there are multiple vftables at the same offset in the class. I noticed in some gcc programs that there seem to be multiples of the same class in the symbol tree when they really should be different classes. It could be that is the problem or there really is one class but code is misidentifying an extra vftable in the class for some reason. It is really hard to diagnose without an example. Is the binary publically downloadable?

[ghidra007]

Do you remember where in the code it was trying to use a negative number of bases?

[ghidra007]

For the negative base issue, I can add a check to make sure the tables are not made in executable memory.

[ghidra007]

Can you replace this code in the processGccRTTI() method (line 231 of ghidra/Ghidra/Features/Decompiler/ghidra_scripts/classrecovery/RTTIGccClassRecoverer.java)

     // find all typeinfo symbols and get their class namespace and create RecoveredClass object
     List<Symbol> typeinfoSymbols = extendedFlatAPI.getListOfSymbolsInAddressSet(
		program.getAddressFactory().getAddressSet(), "typeinfo", true);

with this code:

     // find all typeinfo symbols and get their class namespace and create RecoveredClass object
     AddressSet nonExecutableAddressSet = program.getAddressFactory()
			.getAddressSet()
			.subtract(program.getMemory().getExecuteSet());

     List<Symbol> typeinfoSymbols = extendedFlatAPI.getListOfSymbolsInAddressSet(
		nonExecutableAddressSet, "typeinfo", true);

and tell me if that fixes the first issue? Thanks!

[astrelsky]

Unfortunately it did not. The typeinfoAddresses list in createTypeinfoStructs on line 1353 needs to be filtered.

[ghidra007]

I've updated and will release in one of the next patches. Thanks for the help.

[astrelsky]

I've updated and will release in one of the next patches. Thanks for the help.

I see the commit, will give it a shot tomorrow morning.

[ghidra007]

thanks

[astrelsky]

thanks

Seems to have fixed it. It didn't pick up any rtti though but I forgot to verify if there even was any to begin with 🤣

So there is rtti present. When explicitly running the GccRttiAnalysisScript I noticed that it complained "classname vtable has no typeinfo ref after vtable at address" however the address shown is the address immediately following the end of the vftable instead of the vbtable.

Just to ensure we are on the same page I am referring to things as such (hopefully this makes sense)

symbol: vtable for classname // linker symbol when not stripped
ptrdiff_t array: vbtable
pointer: (typeinfo for classname)*
pointer array: vftable

[0xBEEEF]

@ghidra007
I have now played around with the script for the last few days. I have used an example of @astrelsky that has been super suitable for this. I extracted the test case "LargeInheritance" from his InheritanceTests and compiled it in Visual Studio. You can find the original here, so you can recreate the case as well.

Thereby I noticed the following points, which I find in part still in need of improvement:

If the structures are split and integrated into others, then it comes to unsightly name duplications with the term "vftablePtr". This leads to multiple occurrences of the term "vftablePtr" in a structure, which makes it impossible to recognize the structure in the decompilation and to edit the structure, because the error message "Name "vftablePtr" already exists" always appears.

It would be enough for me if the names here would be based on the class names, similar to the data structures, e.g. W_vftablePtr, W_data, if the class is named W.

This is how it could look like as an example:

Here is another example, this time for class D, which is much simpler in structure.

This is how it looks so far (note again the name duplications):

And this is how it would actually be optimal:

I hope that I have not made any blunders now. My goal is only to improve the overall result. I would be satisfied if the VFTables had a reference, the VBTables would be more or less indifferent to me.

[astrelsky]

I've run into this exact situation in my analyzer with my vtable structs. The deatructors have the same field name as well as pure virtual functions. Your not allowed to do it from the ui but you can from the API.

[ghidra007]

Sorry I didn't get notified about these comments for some reason. Not even in my junk folder which is where they normally end up for some reason. I'll take a look to see what I can do. With the refactor to get the structures more accurate in the decompiler, this was part of the compromise along with making the structures more complicated. Also, if you notice vftables with a number instead of a "for_parent" then the script couldn't figure out the correct parent/ancestor for that vftable and indicates there are issues with the determination of that class structure. There are some complex cases that need more work. This is why the script is prototype and why we are grateful for any feedback that can help us improve it.

[0xBEEEF]

I have a follow-up question about VFTables. In this discussion it was written somewhere above that there would be a trick to use the function "Goto Definition" also for virtual functions, i.e. to get to the calling function relatively effortlessly.

Can someone explain me how this works? Unfortunately I don't find anything about this on the UI, not even in the decompiler window. Would be super handy if I could somehow get to the actual function faster, and without much detours.

[ghidra007]

I'm sorry for the delay. I didn't get notified of your comment for some reason. You can add a reference on the CALL instruction or operand that has one or more references to the possible function(s) you could go to. Then double click the reference to go there (if single one) or pop up list of possibles (if multiple ones). To add a reference, right mouse on the CALL or operand and choose References -> Add/Edit... then choose the plus icon to add a new Memory reference.

[astrelsky]

@ghidra007 constructors do not have a return value. Having the rtti analysis script set one causes an erroneous return (ClassType *)this; at the end of every constructor.

[ghidra007]

The script uses the decompiler to determine whether or not to do this. If the decompiler thinks the return is void it leaves it void. If not, it makes it the this.

[FrankvdStam]

@ghidra007 Not sure if this is the right place/right person to ask but here goes

Ran this script on a bigger program, running into 2 separate issues.

First problem is, the script seems to hang. It was stuck on this for a couple of hours:

I'm not sure where it's getting this definition from. It doesn't exist in the tree. When I go to that address, ghidra doesn't seem to know what it is.

Also, ghidra doesn't let me cancel this task. I have to close ghidra via the project window or kill it with taskmanager in order to cancel it.

I fetched the latest versions of all the files that you've seemingly been working on, and ran it as a custom script. Found that disabling these 2 lines of code would fix this hang by skipping the fixup & analysis (should already be done, I have tons of RTTI_Complete_Object_Locator's in the symol tree)

recoverClassesFromRTTI.fixUpProgram();
analyzeProgramChanges(beforeScriptChanges);

Here:

ghidra/Ghidra/Features/Decompiler/ghidra_scripts/RecoverClassesFromRTTIScript.java

Line 269 in 2d3f68c

recoverClassesFromRTTI.fixUpProgram();

I hoped this would not break anything further down the line, It might have, or it might be a completely separate issue, but I'm running into a crash after about an hour.

Here's a text version of that stacktrace:

java.lang.reflect.InvocationTargetException
	at ghidra.app.plugin.core.analysis.AutoAnalysisManager$AnalysisWorkerCommand.applyTo(AutoAnalysisManager.java:1713)
	at ghidra.app.plugin.core.analysis.AutoAnalysisManager$AnalysisTaskWrapper.run(AutoAnalysisManager.java:688)
	at ghidra.app.plugin.core.analysis.AutoAnalysisManager.startAnalysis(AutoAnalysisManager.java:788)
	at ghidra.app.plugin.core.analysis.AutoAnalysisManager.startAnalysis(AutoAnalysisManager.java:667)
	at ghidra.app.plugin.core.analysis.AutoAnalysisManager.startAnalysis(AutoAnalysisManager.java:632)
	at ghidra.app.plugin.core.analysis.AnalysisBackgroundCommand.applyTo(AnalysisBackgroundCommand.java:58)
	at ghidra.framework.plugintool.mgr.BackgroundCommandTask.run(BackgroundCommandTask.java:102)
	at ghidra.framework.plugintool.mgr.ToolTaskManager.run(ToolTaskManager.java:319)
	at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: ghidra.util.exception.InvalidInputException: Invalid parent namespace specified for new Label symbol: Global
	at ghidra.program.database.symbol.SymbolManager.checkAddressAndNameSpaceValidForSymbolType(SymbolManager.java:3021)
	at ghidra.program.database.symbol.SymbolManager.validateNamespace(SymbolManager.java:2912)
	at ghidra.program.database.symbol.SymbolManager.createCodeSymbol(SymbolManager.java:2746)
	at ghidra.program.database.symbol.SymbolManager.createLabel(SymbolManager.java:2728)
	at ghidra.program.flatapi.FlatProgramAPI.createLabel(FlatProgramAPI.java:451)
	at ghidra.program.flatapi.FlatProgramAPI.createLabel(FlatProgramAPI.java:431)
	at ghidra.program.flatapi.FlatProgramAPI.createLabel(FlatProgramAPI.java:404)
	at classrecovery.RecoveredClassHelper.identifyOperatorNewFunction(RecoveredClassHelper.java:4381)
	at classrecovery.RecoveredClassHelper.findCloneFunctions(RecoveredClassHelper.java:4259)
	at classrecovery.RTTIWindowsClassRecoverer.processConstructorAndDestructors(RTTIWindowsClassRecoverer.java:1321)
	at classrecovery.RTTIWindowsClassRecoverer.createRecoveredClasses(RTTIWindowsClassRecoverer.java:160)
	at RecoverClassesFromRTTIScriptCustom.run(RecoverClassesFromRTTIScriptCustom.java:274)
	at ghidra.app.script.GhidraScript.executeNormal(GhidraScript.java:379)
	at ghidra.app.script.GhidraScript$1.analysisWorkerCallback(GhidraScript.java:361)
	at ghidra.app.plugin.core.analysis.AutoAnalysisManager$AnalysisWorkerCommand.applyTo(AutoAnalysisManager.java:1707)
	... 8 more

Seems to be caused by this: Invalid parent namespace specified for new Label symbol: Global
It's been a while since I've used java but I'm willing to investigate. Gona see if I can setup an IDE and use a debugger to see what is going on. Meanwhile you can tell me why this is happening or have any more information?

[FrankvdStam]

@ghidra007 haha apparently not.
Was wondering, would running this https://github.com/astrelsky/Ghidra-Cpp-Class-Analyzer mess with this script? I'll try again without running that first.

If you need some reproduction, this stuff is in a shared repo. More info here: http://soulsmodding.wikidot.com/tool:main#toc16

Yeah I have the timeouts on 500 seconds, hoping that the results are better. I don't really mind it taking long, I just thought it was getting stuck.

[astrelsky]

@ghidra007 haha apparently not. Was wondering, would running this https://github.com/astrelsky/Ghidra-Cpp-Class-Analyzer mess with this script? I'll try again without running that first.

If you need some reproduction, this stuff is in a shared repo. More info here: http://soulsmodding.wikidot.com/tool:main#toc16

Yeah I have the timeouts on 500 seconds, hoping that the results are better. I don't really mind it taking long, I just thought it was getting stuck.

It shouldn't but because the decompiler is given the first datatype it finds for the class name (sorted by datatype path) it will use the ones from the new scripts.

[ghidra007]

I put in the script description that I recommend running on a clean analysis and that it hasn't been tested on anything but that and that weird results could happen otherwise. So it is possible that could cause some confusion. Thanks for the link so we can try to reproduce.

[emteere]

If you think Ghidra is getting stuck somewhere you can try dumping the threads which can show what area of analysis or of the script might be taking all the time.

First use "jps" to dump the java process ID's.
Then user "jstack " on the one associated with Ghidra.

That should provide a ballpark to see where there might be a badly coded loop, or some other algorithm taking up time.
You might want to do it several times to see if it stays in the same analyzer or area of code.
When you have something you can post the thread dump.

[emteere]

Another way is to use visualvm from visualvm.github.io/.
It is easier to see the accumulated time for various parts of the code, and can attach to a running ghidra at any point in time.
The sampler tab is the easiest to use, and won't slow the program too much. It may take a little time to learn if you have never used visualvm.

[ghidra007]

To fix the stack trace replace RecoverClass.class line 524
which was
computedClassStructure.replaceAtOffset(offset, pointer, pointer.getLength(),fieldName, comment);

with:

if (EditStructureUtils.hasEnoughUndefinedsOfAnyLengthAtOffset(computedClassStructure, offset, pointer.getLength(), monitor) ||
		pointer.getLength() <= currentComponent.getLength()) {

		computedClassStructure.replaceAtOffset(offset, pointer, pointer.getLength(),fieldName, comment);
}

Thanks for helping to find this issue.

[FrankvdStam]

Keep in mind that this binary has issues importing/disassembling. Maybe there is no solution. I'm fine with a solution that skips certain classes if they fail, as long as 1 broken class doesn't bring the rest down with it 😄

[ghidra007]

I almost have something for you to try so it should at least not blow up in that latest method. No guarantee what you end up with is any good since your binary has issues which are probably causing decompiler errors which are probably causing erroneous class data type information to be created since that data creation relies on the decompiler to figure out the class data. I'm running some tests and if they go well I'll post a fix for you to try.

[ghidra007]

I've reworked the addClassVftables method in RecoveredClassHelper. Here is the code to replace it with if you want to test the fix:

/**
* Method to add/overwrite class vftable pointers to the given class structure
* @param classStructureDataType the class structure data type
* @param recoveredClass the given recovered class
* @param vfPointerDataTypes the map of addresses/vftables, a null should be passed to indicate
* no known vftables for the given class.
* @return the modified structure with the vftable pointers added or an unchanged structure if
* the vftable map is null or if the given class's offset to vftable map is empty
* @throws CancelledException if cancelled
* @throws IllegalArgumentException if there are issues modifying the structure
*/
protected Structure addClassVftables(Structure classStructureDataType,
RecoveredClass recoveredClass, Map<Address, DataType> vfPointerDataTypes)
throws CancelledException, IllegalArgumentException {

	Map<Integer, Address> classOffsetToVftableMap = recoveredClass.getClassOffsetToVftableMap();
	Set<Integer> classVftableOffsets = classOffsetToVftableMap.keySet();

	if (vfPointerDataTypes == null || classVftableOffsets.isEmpty()) {
		return classStructureDataType;
	}

	// If the map is not complete, the class structure will contain incomplete information so 
	// put out a debug message to indicate this issue
	if (!isClassOffsetToVftableMapComplete(recoveredClass)) {
		Msg.debug(this,
			"class vftable offset map for " + recoveredClass.getName() + " is not complete");
	}

	// iterate over the set of offsets to vftables and either add to undefined area or overwrite 
	// the parent class structures with the class vftable pointer then replace the rest of the 
	// parent structure with its internal components
	for (Integer offset : classVftableOffsets) {
		monitor.checkCanceled();

		Address vftableAddress = classOffsetToVftableMap.get(offset);

		int vftableOffset = offset.intValue();

		DataType classVftablePointer = vfPointerDataTypes.get(vftableAddress);

		if (classVftablePointer == null) {
			continue;
		}


		// loop until the vftable pointer is added
		// if there is room and the component offset is not a structure, replace with vftablePtr
		// otherwise, within the range from top of containing component to the end of where the
		// vftable it to replace, clear component(s) or replace structure(s) with internal
		// components and loop until can replace with the vftable pointer
		while (true) {

			monitor.checkCanceled();

			// if enough empty bytes or can grow the structure - add class vftable pointer
			boolean addedToStructure =
				EditStructureUtils.addDataTypeToStructure(classStructureDataType, vftableOffset,
					classVftablePointer, CLASS_VTABLE_PTR_FIELD_EXT, monitor);
			if (addedToStructure) {
				break;
			}

			// if the component at the offset is the start of the component, and the component
			// isn't a structure and if the component is big enough to be replaced, or there 
			// are enough undefined that can be replaced then replace
			DataTypeComponent componentAt =
				classStructureDataType.getComponentAt(vftableOffset);

			if (componentAt != null && !(componentAt.getDataType() instanceof Structure) &&
				(componentAt.getLength() >= classVftablePointer.getLength() ||
					EditStructureUtils.hasEnoughUndefinedsOfAnyLengthAtOffset(
						classStructureDataType, vftableOffset, classVftablePointer.getLength(),
						monitor))) {

				EditStructureUtils.clearLengthAtOffset(classStructureDataType, vftableOffset,
					classVftablePointer.getLength(), monitor);
				classStructureDataType.replaceAtOffset(vftableOffset, classVftablePointer,
					classVftablePointer.getLength(), CLASS_VTABLE_PTR_FIELD_EXT, "");
				break;
			}

			// otherwise if in middle of a containing dt then loop until all structs in 
			// range are expanded and other items are cleared
			DataTypeComponent currentComponent =
				classStructureDataType.getComponentContaining(vftableOffset);

			int currentOffset = currentComponent.getOffset();
			int endOffset = vftableOffset + classVftablePointer.getLength();
			while (currentComponent != null && currentOffset < endOffset) {

				int nextOffset = currentComponent.getEndOffset() + 1;
				// if there is a structure at the offset, replace it with its pieces 
				if (currentComponent.getDataType() instanceof Structure) {
					DataType currentDT = currentComponent.getDataType();
					Structure internalStruct = (Structure) currentDT;
					expandInternalStructure(classStructureDataType, internalStruct,
						currentOffset);
				}
				// if not a structure then clear it
				else {
					classStructureDataType.clearAtOffset(currentOffset);
				}
				currentOffset = nextOffset;
				currentComponent = classStructureDataType.getComponentAt(currentOffset);
			}
		}
	}
	return classStructureDataType;
}

/**
 * Method to replace an internal structure into individual parts within the containing structure
 * @param structure the containing structure
 * @param internalStruct the internal structure
 * @param offset the offset of the internal structure
 */
private void expandInternalStructure(Structure structure, Structure internalStruct,
		int offset) {

	DataTypeComponent[] components = internalStruct.getComponents();

	// if there is an empty structure at the offset, clear it which will replace
	// it with an undefined data type of size 1
	if (components.length == 0) {
		structure.clearAtOffset(offset);
		return;
	}
	// if non-empty, replace the structure with its components
	for (DataTypeComponent component : components) {

		int innerOffset = component.getOffset();

		// add indiv components of internal structure to the outer structure
		structure.replaceAtOffset(offset + innerOffset, component.getDataType(),
			component.getLength(), component.getFieldName(), "");
	}
	return;
}

[FrankvdStam]

Ran to completetion, thank you so much! 😄

This UI thingy stays open after it finishes and clicking cancel doesn't really do anything - I think I introduced this bug though, with my own changes.

[ghidra007]

Glad it finally finished.

[astrelsky]

Here's a bit of an update. The demangler will correctly demangle the external mangled symbols but not update the symbol at the internal address pointing to the external address. I was hoping that dealing with that use-case and adding a check for the PE mingw's in the class recovery script would be all that was needed to fix the script. It certainly got it farther. However, another issue I'm seeing in the mingw's are that the external relocations are being treated like they are PE ones and I think they should possibly be treated more like the unsupported ELF external relocations. I'm testing on one of your mingw main programs and the typeinfo structures that normally point to the EXTERNAL memory section and there is Ghidra logic built in to not actually reference offsets normally are in the mingw being treated as normal offsets into a normal data memory block so are pointing to the wrong address in that table. This is preventing the script from identifying which of the special typeinfo's are being pointed to. Instead, it appears to the script that they are pointing to something unrelated so processing stops. I'm not sure if it is related to #761 but could be. In any case, this issue is keeping me from being able to handle mingw rtti's correctly right now and I'm not seeing an easy temporary hack around this issue. There will probably be other issues we will bump into when figuring out how to treat these files once the correct compiler spec is created. The various analyzers like demangler, exception handler, and rtti, and probably others, will have to be updated to check for the compiler spec and then do the right thing. In the demangler case, it should be simple. Same with class recovery once the relocations are correct. Hopefully the rest will be simple too.

I'm moving this here. It was convenient at the time to put it in the commit so you knew exactly what I was referring to but it doesn't play very nice with the mobile app.

Anyway it's definitely not related to #761. I see a handful of problems that I do not recall seeing before. For one the relocation table is empty which is rather annoying because I use it when the special typeinfo are external. I do not recall if this is normal for a PE or not though. Also the symbols at the address for the vtables are PTR_vtable_0041c74c instead of _ZTVN10__cxxabiv120__si_class_type_infoE for example. I don't recall ever seeing this problem in the past so I don't have an immediate work around either 😅.

[astrelsky]

Verified that I was looking at the ones marked static that we got from your test files but they are definitely different than the one you attached. Not sure if you generated new ones after we pulled them or what. Importing directly from your tar file using our file system importer so they are in the folders laid out by your naming system.

I'll have to go check that then. I may have fixed a mistake in the makefile and not pushed it or released it or something.

[astrelsky]

Verified that I was looking at the ones marked static that we got from your test files but they are definitely different than the one you attached. Not sure if you generated new ones after we pulled them or what. Importing directly from your tar file using our file system importer so they are in the folders laid out by your naming system.

It seems you are correct. There must have been an issue with making them static last time I built them. I went to build them again and now I can't figure out why none of them will build 😅

[ghidra007]

Bummer. Glad you figured out the discrepancy though. Good luck figuring out the build issues!

[astrelsky]

Bummer. Glad you figured out the discrepancy though. Good luck figuring out the build issues!

Turns out all I needed was some early morning clarity and a fresh cup of coffee. I published another release and ensured the static ones are indeed static.

[ghidra007]

Great! Will take a look.

[0xBEEEF]

I have now played around a bit with pharos. The tool also does some of what you try in your scripts, only it goes a bit beyond that. I have recognized another quite important functionality. At the beginning of the analyses you specify the memory reservation method and the memory release method. Here are the descriptions from the manual:

--new-method=ADDRESS, -n=ADDRESS
Mark a specific address as an implementation of the new() operator. Usually new() implementations are detected using a combination of import names and function hashes, but not all new() implementations are automatically recognized. If no new implementations are detected, it can dramatically affect the detection of dynamically allocated objects. This option may be specified multiple times.

--delete-method=ADDRESS
Mark a specific address as an implementation of the delete() operator. Usually delete() implementations are detected using a combination of import names and function hashes, but not all delete() implementations are automatically recognized. If no delete implementations are detected, it can dramatically affect the detection of destructor methods. This option may be specified multiple times.

It is quoted 1:1, you can find the original document here. This allows the program to find the presumed locations where a constructor or destructure is called. I found this quite interesting, plus it uses these locations to detect the sizes of the class structures. This works surprisingly well.

Wouldn't this be something for your scripts, @ghidra007?

I noticed in some examples that the class size detection sometimes works better, sometimes worse, in pharos the detection here was usually much better and more accurate. I think this could be improved mainly by providing the above methods the quality even better.

[ghidra007]

Thanks for the info. Our script does detect operator new and delete methods and uses them to help determine deleting destuctor and clone functions. Some improvements in their detection were merged into our master branch and should be available to pull if you want to try it out. The script currently doesn't update the labels of the found functions but that is on the list. Sometimes it is difficult to automate figuring this out due to what other functions they thunk. This update offers a slight improvement on determining correct constructor / destructor functions. One thing the script currently doesn't do is use this information to help determine the class structure size. That is on the list to do. The script instead relies on the decompiler structure recovery to do this.

[ghidra007]

Unless they haven't updated their documentation on their main github page, it looks like they only currently handle 32-bit MS Visual Studio x86 c++ files.

[0xBEEEF]

Thank you for the detailed answer. Without having looked into every single line of code you wrote: What exactly do you currently use to determine the sizes of certain classes? I had some time ago (possibly already corrected, would have to try again) a few cases where the size determined by you was not exactly right compared to the specified size in the memory reservation function. The deviations were sometimes small (4 to 8 bytes) and rarely larger. I then compared this with pharos, and here it came to significantly less (of course there was also) deviations in the size.

[ghidra007]

If the program has pdb or dwarf information for a particular class structure, I use that. If not, I use the decompiler's FillOutStructureCmd for each known constructor and destructor and combine the information from them to make a class structure. I also use inherited class structure information. If the script doesn't get the constructor/destructor information right or if the decompiler doesn't have all of it's information right, this could cause the issues. In some contrived examples I have seen where there are class data members but they are not initialized in a class constructor or cleared in a class destructor, I have also seen these small discrepancies. It is definitely on the list to make improvements to this. Thanks for your patience and for identifying issues so we can continue to improve.

[0xBEEEF]

@ghidra007
I would like to revive this discussion thread once. It has become surprisingly quiet. The last commits in the Master Branch to the RTTI scripts were several months ago, at the beginning of June to be exact.

Has the topic fallen asleep, is it still being actively worked on, or is it completely dead?

I think it's kind of a pity, there was so much energy put into this topic, and now you don't hear anything about it anymore.

Is it somehow possible to get a short status report here, how things stand, and whether it is still worthwhile to stay tuned, or is this wasted lifetime?

[ghidra007]

Sorry for the crickets. There has been experimentation with doing class recovery when there are classes with no RTTI. This involves figuring out valid vftables, putting them in correct classes when there are more than one in a class, figuring out parents, etc..

[0xBEEEF]

That sounds really brilliant! Glad to hear that you are still active on the subject. I was just a little scared that it has become so terribly quiet around the topic in recent months.

Will there be prototypes for testing here soon, or will it only be available for the release of the 10.2 version (whenever that is)?

[ghidra007]

It is going very well but taking a bit longer than expected, unfortunately. I definitely would like some more testers, though, and plan to push out a prototype for that very reason once it is to that point.

[ghidra007]

Currently it is focused on Windows programs but am hoping it will extend well to other program types.

[FeeeeK]

It seems that if a program contains multiple classes with the same name in an anonymous namespace, the script will fail with the error trying to add different vftable address (old: ....... new: .......) to same offset 0

[ghidra007]

Tried running the script again on 11.2.1 and it seems that the problem with trying to add different vftable address to same offset 0 still exists. Even though the vftables now have different anon namespaces: _anon_8AF2C10D::Example::vftable and _anon_4C8542B5::Example::vftable

Did you run a completely new analysis on a fresh import of the program before running the script? Is it happening in the same class as before or somewhere else? Do the anon classes each have their own vftable now that the other issue is fixed?

[FeeeeK]

1. Yes
2. Yes, in the same class
3. Not sure what you mean, that's what I have in the listing view:

                             ***********************************************************************
                             * meta pointer for _anon_4C8542B5::Example::vftable           *
                             ***********************************************************************
                             _anon_4C8542B5::Example::vftable_meta_ptr
        140c31608       addr       _anon_4C8542B5::Example::RTTI_Complete_Object_Locator


                             ***********************************************************************
                             * const _anon_4C8542B5::Example::vftable                      
                             ***********************************************************************
                             Example::`vftable'                       XREF[2]:     ExampleFunc:1401e0920
                             _anon_4C8542B5::Example::vftable                      ExampleFunc:1401e0927
       140c31610        addr[20]

and

                             ***********************************************************************
                            * meta pointer for _anon_8AF2C10D::Example::vftable           *
                            ***********************************************************************
                            _anon_8AF2C10D::Example::vftable_meta_ptr
       140c32588       addr       _anon_8AF2C10D::Example::RTTI_Complete_Object_Locator


                            ***********************************************************************
                            * const _anon_8AF2C10D::Example::vftable                      
                            ***********************************************************************
                            Example::`vftable'                       XREF[2]:     ExampleFunc2:1401e3644
                            _anon_8AF2C10D::Example::vftable                      ExampleFunc2:1401e364b
      140c32590        addr[20]

[ghidra007]

Ok thanks for the info.

[ghidra007]

I am able to reproduce. I apologize for not making sure the fix to the namespaces fixed the original issue before the patch went out. I thought I had done so.

[ghidra007]

I have figured out the issue. The secondary symbols that are still in the Example namespace that is not in either anonymous namespace are causing the issue. You can fix manually until I get a fix out. If you filter in the Symbol Tree for Example and find the folder that has the bad symbols and delete that folder you should be good. Be careful not to delete the good ones.