Fixes for SLA file parsing bugs uncovered during fuzzing

[mnemonikr]

Why is this change being proposed?

I was fuzzing the .sla parser in the decompiler and resolved a number of issues uncovered. I've included all of the fixes in this PR, but if preferred I can split this into multiple PRs. I tried to keep each commit self-contained to the issue I was resolving for easier reviewing.

Heap buffer overflows

These all triggered Address Sanitizer bug reports prior to being fixed:

1. There is a buffer overflow in PackedDecode::advancePosition due to the signed skip parameter, which is read from the .sla file. If negative, can result in a read out of bounds. This was changed to unsigned.
2. Identifiers read from the file and used as indexes should be validated to not exceed the container size. These included the symbol list, symbol scope table, address spaces, and constructors.

Null pointer dereferences

1. If endIngest is called without any additional data, calls to decode will result in null deref when getByte tries to deref endPos.current which isn't set to point to the input stream (since there isn't one allocated). This can be triggered by an .sla file with just the header. I changed endIngest here to throw an exception in this case.
2. Symbol list and symbol scope tables updated to ensure their entries are non-null before deleting in destructor.
3. Pattern expression decoder returned null if invalid, but none of the callers checked for it. Changed this to an exception.

Memory leaks

These were issues identified by Leak Sanitizer and largely followed the pattern

TypeToDecode * localVar = new TypeToDecode();
localVar->decode(...);
owner->var = localVar;

If an issue occurred in decode, the localVar type would never be destroyed. These were largely mitigated with the use of std::unique_ptr locally to ensure the value would be destroyed on exception. The unique pointer is released prior to returning to prevent unwanted destruction on success.

The other common memory leak was overwriting the same value if decoding the same id multiple times. These scenarios were turned into exceptions. However, if there is a usecase for these, can update to change to free the previous value and allow overwriting instead.

Additional changes

The following files were not technically a part of the fuzz run but exhibited the same decoding pattern that resulted in the memory leaks elsewhere. I've included patches for these that should resolve the same style of memory leaks on decoding failure.

• architecture.cc
• funcdata.cc
• modelrules.cc

[jobermayr]

Please add:
#include <memory>

in:

Ghidra/Features/Decompiler/src/decompile/cpp/architecture.cc
Ghidra/Features/Decompiler/src/decompile/cpp/funcdata.cc
Ghidra/Features/Decompiler/src/decompile/cpp/modelrules.cc
Ghidra/Features/Decompiler/src/decompile/cpp/override.cc
Ghidra/Features/Decompiler/src/decompile/cpp/semantics.cc
Ghidra/Features/Decompiler/src/decompile/cpp/slghpatexpress.cc
Ghidra/Features/Decompiler/src/decompile/cpp/slghpattern.cc
Ghidra/Features/Decompiler/src/decompile/cpp/slghsymbol.cc
Ghidra/Features/Decompiler/src/decompile/cpp/translate.cc

to fix compile errors like:

/tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/override.cc:368:12: error: no member named 'unique_ptr' in namespace 'std'
  368 |       std::unique_ptr<FuncProto> fp(new FuncProto());
      |       ~~~~~^
/tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/override.cc:368:23: error: 'FuncProto' does not refer to a value
  368 |       std::unique_ptr<FuncProto> fp(new FuncProto());
      |                       ^
/tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/include/ghidra/fspec.hh:1353:7: note: declared here
 1353 | class FuncProto {
      |       ^
/tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/override.cc:368:34: error: use of undeclared identifier 'fp'
  368 |       std::unique_ptr<FuncProto> fp(new FuncProto());
      |                                  ^
/tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/override.cc:369:7: error: use of undeclared identifier 'fp'
  369 |       fp->setInternal(glb->defaultfp,glb->types->getTypeVoid());
      |       ^
/tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/override.cc:370:7: error: use of undeclared identifier 'fp'
  370 |       fp->decode(decoder,glb);
      |       ^
/tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/override.cc:371:37: error: use of undeclared identifier 'fp'
  371 |       insertProtoOverride(callpoint,fp.release());
      |                                     ^

[mnemonikr]

Apologies, updated! I've been running make test locally to confirm builds but should've added the include when bringing in the new usage of unique_ptr.