fix decompiler display of char array #3001

Escapingbug · 2021-05-04T05:34:36Z

When a char array is splitted out of a long char array, the decompiler's display does not respect the defined data and display the whole string.
This happens a lot when decompiling Rust.

For example, before this patch:

After this patch:

According to this comment, this issue should be resolved in 9.2. But when I try it, it is still a problem. This should fix it.

Previous PR(#3000 ) contains some commits not belong to this patch. It should all be OK now.

astrelsky · 2021-05-04T11:12:41Z

Ghidra/Features/Decompiler/src/main/java/ghidra/app/decompiler/DecompileCallback.java

-			if (data.getDataType() instanceof AbstractStringDataType) {
-				// There is already a string here.  Use its configuration to
-				// set up the StringDataInstance
+			if (data.getDataType() instanceof AbstractStringDataType


Suggested change

if (data.getDataType() instanceof AbstractStringDataType

if (data.hasStringValue())

I believe this method is more appropriate here.

ps I'm on my phone so line selections may be wierd.

Maybe it is more appropriate. But for now I can not be 100% sure if data.hasStringValue() returns true, the only possible data types are AbstractStringDataType and Array.

And later, the getStringDataInstance method are not abstracted out of DataType, i.e, there's no interface contains such a method.
Thus, current implementation checks the type with instanceof one by one (this it the implementation before this patch anyway).
If we use hasStringValue() here, I suppose we have two options:

figure out what are the potential types and type check and cast them one by one then call getStringDataInstance method.

do some more abstraction, use interface like StringableDataType and contains method of getStringDataInstance.

And for now, I'm not confident enough I can do either without any help. (I'm also working on some more fixings related to better Rust decompilation. I tend to make them work first)

Try the following. It appears that the work is already done in StringDataInstance.
You can actually skip the entire if (data == null) block and instead do the following:

StringDataInstance stringInstance = StringDataInstance.getStringDataInstance(data); if (stringInstance == StringDataInstance.NULL_INSTANCE) { // previous if (data != null) else block contents L1348-L1377 }

As a side note, when you have a Data instance and Data.hasStringValue() returns true you can do the following:

public static String getStringFromData(Data data) { if (data.hasStringValue()) { return (String) data.getValue(); } return ""; }

With that said, if you want to keep it the way it is because it introduces the smallest amount of changes I think that is fine. I will probably comb through this file to simplify the logic here and check if there are other simplifications that could be made this weekend.

Thanks for the advice! I have pushed the new code adopting that option.

I'm terribly sorry. I made a mistake. Removing the if (data != null) block completely breaks what the patch was intended to fix. It should go back to how that was. However, it couble be skipped if data.getAddress().equals(addr), but I no longer trust myself looking at this with tired eyes.

Ah! I just noticed it! I will make it back. You are right, current patch introduces incorrect starting address problem

astrelsky · 2021-05-04T11:14:58Z

Ghidra/Features/Decompiler/src/main/java/ghidra/app/decompiler/DecompileCallback.java

@@ -1336,13 +1335,20 @@ public StringData getStringData(String addrString, String dtName, String dtId) {
 				}
 				length -= diff;
 				MemoryBufferImpl buf = new MemoryBufferImpl(program.getMemory(), addr, 64);
-				stringInstance = dataType.getStringDataInstance(buf, settings, length);
+				if (data.getDataType() instanceof AbstractStringDataType) {


Just in case, I don't think the comment above applies here. This should be correct.

astrelsky · 2021-05-04T11:17:52Z

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/Array.java

+	 * @return a String if it is an array of chars; otherwise null.
+	 */
+	public default StringDataInstance getStringDataInstance(MemBuffer buf, Settings settings, int length) {
+		if (!buf.getMemory().getAllInitializedAddressSet().contains(buf.getAddress())) {


There might be a more appropriate way to check if the address is initialized memory other than collecting the entire set. You could probably peek at the implementation of getAllInitializedAddressSet for a hint here.

Actually, this is copied directly from the getArrayValue down below. If we need a more appropirate way of do this, I think it maybe better to have another PR do this? Since actually.. I haven't checked and totally understood how this part works, but I think they share the same logic so I copied that here.

I see. There is Address.isLoadedMemoryAddress() but I don't know if this means it is initialized or not. Any other alternative I can think of is more than one line.

Are these 2 other classes necessary for this fix?

Sorry this is the thing missing when I clean up code when the data != null block has been changed. This should be fine now.

dev747368 · 2021-05-05T23:16:14Z

Ghidra/Features/Decompiler/src/main/java/ghidra/app/decompiler/DecompileCallback.java

 			// There is no string and/or something else at the address.
 			// Setup StringDataInstance based on raw memory
 			DataType dt = dtmanage.findBaseType(dtName, dtId);
+			AbstractStringDataType dataType = null;


This local var (dataType) needs a better name, ie. tmpStringDataType

This actually is the original name... I just followed the previous version. I thought it was better to keep the commit minimum.

dev747368 · 2021-05-05T23:45:50Z

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/Array.java

+	 * @return a String if it is an array of chars; otherwise null.
+	 */
+	public default StringDataInstance getStringDataInstance(MemBuffer buf, Settings settings, int length) {
+		if (!buf.getMemory().getAllInitializedAddressSet().contains(buf.getAddress())) {


Are these 2 other classes necessary for this fix?

Fixes issue reported in PR #3001

fix decompiler display of char array

8e5ef66

ryanmkurtz added the Feature: Decompiler label May 4, 2021

astrelsky reviewed May 4, 2021

View reviewed changes

Anciety added 2 commits May 5, 2021 10:53

clean up code

3e177f7

clean up not useful null check

0b2c06c

dev747368 reviewed May 5, 2021

View reviewed changes

Anciety added 2 commits May 6, 2021 10:04

clean up unused code

7554ffb

fix for wrong starting address

1ae4fb5

Escapingbug mentioned this pull request May 16, 2021

Rust decompiler improvement StarCrossPortal/sleighcraft#12

Closed

3 tasks

Escapingbug mentioned this pull request May 25, 2021

Better Rust support StarCrossPortal/ghidracraft#9

Open

4 tasks

fix incorrect initial value problem in DecompileCallback

cb21998

ryanmkurtz added this to the 10.3 milestone Nov 3, 2022

ryanmkurtz assigned dev747368 Nov 3, 2022

ryanmkurtz added the Reason: Internal effort This will be solved internally label Nov 3, 2022

ryanmkurtz pushed a commit that referenced this pull request Nov 8, 2022

GP-2704 fix decompiler rendering of array string offcuts

2cbbc24

Fixes issue reported in PR #3001

ryanmkurtz closed this in f9406b0 Nov 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix decompiler display of char array #3001

fix decompiler display of char array #3001

Escapingbug commented May 4, 2021

astrelsky May 4, 2021

Escapingbug May 4, 2021

astrelsky May 4, 2021 •

edited

Loading

Escapingbug May 5, 2021

astrelsky May 6, 2021 •

edited

Loading

Escapingbug May 6, 2021

astrelsky May 4, 2021

astrelsky May 4, 2021

Escapingbug May 4, 2021

astrelsky May 4, 2021

dev747368 May 5, 2021

Escapingbug May 6, 2021

dev747368 May 5, 2021

Escapingbug May 6, 2021

Escapingbug May 6, 2021

dev747368 May 5, 2021

	if (data.getDataType() instanceof AbstractStringDataType
	if (data.hasStringValue())

fix decompiler display of char array #3001

fix decompiler display of char array #3001

Conversation

Escapingbug commented May 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrelsky May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrelsky May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrelsky May 4, 2021 •

edited

Loading

astrelsky May 6, 2021 •

edited

Loading