NTRGhidra icon indicating copy to clipboard operation
NTRGhidra copied to clipboard

[SDK decompression] IndexOf always returns -1

Open BrokenR3C0RD opened this issue 4 years ago • 3 comments

Describe the bug The ARM9#IndexOf function never modifies the index variable, meaning it always returns -1 and never finds the moduleparams, leaving compressed and unusuable data in its place.

To Reproduce Steps to reproduce the behavior:

  1. Try loading an SDK-compressed NDS file.

Expected behavior It should decompress the SDK-compressed files.

Screenshots N./A

Desktop (please complete the following information): N/A

Additional context N/A

BrokenR3C0RD avatar Jan 08 '21 18:01 BrokenR3C0RD

I believe this is actually a case of leftover functions in the ARM9.java, as IndexOf() is never used in practice. Thanks for opening this issue as it is a good opportunity to clean and improve the code.

Currently, the decision to decompress or not is based on user input. The user is asked whether the file being loaded is a commercial ROM or not (a homebrew, for example).

The current flow is the following:

NTRGhidraLoader.java: asks the user to choose decompression or not if decompression is chosen --> NDS.java#GetDecompressedARM9() is called

This method checks the static footer (so here really, is where the tool could automatically detect whether a given file has compression or not, without having to rely on user input.).

If the NitroFooter is not null then ARM9.Decompress(MainRom, StaticFooter._start_ModuleParamsOffset); is called.

The ModuleParamsOffset is read from the NitroFooter and passed to the Decompress() method with the following signature: Decompress(byte[] Data, int _start_ModuleParamsOffset) which does not need to use IndexOf() to correctly decompress the data.

public class NitroFooter
	{
		public int NitroCode;
		public int _start_ModuleParamsOffset;
		public int Unknown;
		
		public NitroFooter() { }
		public NitroFooter(BinaryReader er) throws IOException
		{
			NitroCode = er.readNextInt();
			_start_ModuleParamsOffset = er.readNextInt();
			Unknown = er.readNextInt();
		}
		
	}

TLDR; As long as the user chooses the correct option when asked, the loader works correctly. Despite this, there is a lot of room for improvement as the loader could detect if a ROM has compression or not, and act accordingly, without user interaction.

We should start fixing the indexOf() method

pedro-javierf avatar Jan 09 '21 11:01 pedro-javierf

I do have to note that I did have to fix this bug in a local copy of the code in order to decompress a certain application, and without it it ended up ignoring the entire process. I'm sadly away from my computer at the moment but fixing IndexOf is rather trivial, and actually saves a lot of complexity.

Right now, the function stores an index (is supposed to) when it finds a match, and then breaks and returns that index. It would be much more straightforward to simply return i when the match is found and return -1 at the end of the file.

BrokenR3C0RD avatar Jan 09 '21 14:01 BrokenR3C0RD

Interesting, which file were you loading? So far I'm aware NTRGhidra has been tested with over a dozen of games and a bunch of homebrews as well, and the decompressed data for the games tested again a manually-decompressed sample to compare, with good results.

And yes, the indexOf should be trivial to rewrite and would save a lot of lines of code, so you're welcome to contribute whenever possible! Having it working could make possible to remove other parts of code and bring several other optimizations.

pedro-javierf avatar Jan 09 '21 15:01 pedro-javierf