System.NullReferenceException: 'Object reference not set to an instance of an object.'

[spacehamster]

I get the following exception while trying to AsmHook a large number of functions

System.NullReferenceException
  HResult=0x80004003
  Message=Object reference not set to an instance of an object.
  Source=System.Memory
  StackTrace:
   at System.SpanHelpers.CopyTo[T](T& dst, Int32 dstLength, T& src, Int32 srcLength) in E:\A\_work\89\s\corefx\src\System.Memory\src\System\SpanHelpers.cs:line 45
   at System.Span`1.TryCopyTo(Span`1 destination) in E:\A\_work\89\s\corefx\src\System.Memory\src\System\Span.Portable.cs:line 319
   at System.Span`1.CopyTo(Span`1 destination) in E:\A\_work\89\s\corefx\src\System.Memory\src\System\Span.Portable.cs:line 295
   at Reloaded.Hooks.Internal.Patch.ApplyUnsafe() in C:\Files\Code\Hades\Reloaded.Hooks\Source\Reloaded.Hooks\Internal\Patch.cs:line 65
   at Reloaded.Hooks.AsmHook.Activate() in C:\Files\Code\Hades\Reloaded.Hooks\Source\Reloaded.Hooks\AsmHook.cs:line 191

I have created a test case that is able to reproduce the exception here https://github.com/spacehamster/Reloaded.Hooks/commit/0b700248b0b8a1269ed0bb647c09e168eeb27b0e

It appears that Patch is trying to write to a Span where the _address member passed into the constructor has a value of zero here https://github.com/Reloaded-Project/Reloaded.Hooks/blob/26cf3c361de015e178815bd93534892a42b16938/Source/Reloaded.Hooks/Internal/Patch.cs#L36

and the address is zero because Utilities.AssembleAbsoluteJump is writing to the buffer for some reason and so the buffer does not contain the required space when buffer.Add(jmpToHook, codeAlignment); is called and so it returns a IntPtr.Zero https://github.com/Reloaded-Project/Reloaded.Hooks/blob/26cf3c361de015e178815bd93534892a42b16938/Source/Reloaded.Hooks/AsmHook.cs#L122

[Sewer56]

Deduction sounds good and thank you for the test case.

If I were to have a quick guess, the required buffer size is miscalculated by a tiny amount, will double check when I can.

https://github.com/Reloaded-Project/Reloaded.Hooks/blob/f6d8e0609f6ef1de5ec850cfbad2ba012bfc8627/Source/Reloaded.Hooks/AsmHook.cs#L103

The problem is specifically that the original ASM code is rewritten to be relative to a new address (i.e. jumps, calls etc. are adjusted since they are relative to current address). This address needs to be known ahead of time, which creates a problem as I need to know the size of the rewritten code but I can't rewrite the code without knowing the address.

My guess is either the initial predicted size is incorrect due to my programming error or the code in Iced which rewrites the existing code to a new address is increasing the length of the code. I'd recommend comparing the predicted size against the final size, while taking alignment into account.

My personal situation has been a bit weird lately so I might not be able to address this immediately for now but I'll bookmark this issue.

[spacehamster]

I did notice that MakeOriginalStub was consuming more bytes then expected, but the fact that Utilities.AssembleAbsoluteJump was consuming any bytes at all seems very surprising to me as there is no indication that it should from simply looking at the function. I do not think I can diagnose the buffer size calculations without understanding that issue.

alignmentRequiredBytes: 12
stubEntrySize: 7
stubHookSize: 27
stubOriginalSize: 15
requiredSizeOfBuffer: 61
buffer.Properties.Remaining: 72

//codeAlignment. Consumes 0 bytes. Expected 3 bytes max. 72 bytes remaining.
buffer.SetAlignment(codeAlignment);

//stubHookSize. Consumes 27 bytes. Expected 27 bytes. 72 bytes remaining
IntPtr hookStubAddr     = MakeHookStub(buffer, patcher, asmCode, originalFunction, jumpBackAddress, behaviour);

//codeAlignment. Consumes 1 byte. Expected 3 bytes max. 45 bytes remaining
buffer.SetAlignment(codeAlignment);

//stubOriginalSize. Consumes 23. Expected 15 bytes. 44 remaining
IntPtr originalStubAddr = MakeOriginalStub(buffer, patcher, originalFunction, jumpBackAddress);

//??? Consumes 9 bytes. Expected 0 bytes. 21 bytes remaining
byte[] jmpToOriginal = Utilities.AssembleAbsoluteJump(originalStubAddr, _is64Bit);

//???. Consumes 8 bytes. Expected 0 bytes. 12 bytes remaining
byte[] jmpToHook     = Utilities.AssembleAbsoluteJump(hookStubAddr, _is64Bit);

//stubEntrySize. Consumes 0 bytes. Expected  7 bytes + 3 bytes max alignment. 4 bytes remaining.
IntPtr entryStubAddr = buffer.Add(jmpToHook, codeAlignment);

[Sewer56]

Yeah seems like the bug is right there and it should be a fairly easy fix. It seems I was careless and didn't take into account possible buffer use by Utilities.AssembleAbsoluteJump.

One of the library's goals is interoperability with other libraries and hooks. While Reloaded.Hooks can reliably patch existing hooks, it can't guarantee other hooks will play nice. So what Reloaded.Hooks does is use absolute jumps which always jump to the same location regardless of the address. since all jumps by default are relative. To do this it needs to allocate a pointer to the target at a fixed known location (within 32-bit address range, this is an x86 limitation).

This is what makes Utilities.AssembleAbsoluteJump consume 8 bytes on x64 and 4 bytes on x86 as it looks for a buffer to place the pointer in, which can be the same buffer and this was unaccounted for.

The fix should look fairly simple:

// Place under alignmentRequiredBytes
int pointerRequiredBytes = (_is64Bit ? 8 : 4) * 2; // 2 calls to AssembleAbsoluteJump

// Down the road, add this size to the required buffer size.
int requiredSizeOfBuffer = stubEntrySize + stubHookSize + stubOriginalSize + alignmentRequiredBytes + pointerRequiredBytes;

Feel free to add this to your fork and throw a pull request.

[spacehamster]

MakeOriginalStub also has a call to Utilities.AssembleAbsoluteJump that was not accounted for either, which would explain why it consumes 8 more bytes then was expected. I'll have a go at making a PR. Btw, how does Utilities.AssembleAbsoluteJump work with alignment, does any extra space need to be reserved for AssembleAbsoluteJump alignment?

[spacehamster]

I'm not sure if this suggestion is helpful, ignore it if its stupid, but i wonder if Utilities.FindOrCreateBufferInRange should not return buffers that have an lock active in order to prevent unexpected writes to a buffer like that in the future?

[Sewer56]

I'm not sure if this suggestion is helpful, ignore it if its stupid, but i wonder if Utilities.FindOrCreateBufferInRange should not return buffers that have an lock active in order to prevent unexpected writes to a buffer like that in the future?

The suggestion is good; however the whole idea of the underlying library (Reloaded.Memory.Buffers) is to efficiently store memory that is not to be freed; implementing this kind of strategy would lead to the creation of buffers where they may not be strictly necessary.

On a related note; sorry for the wait. PR looks good and I merged it; an updated package should be up now and running.