HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Hook COM Object and get data from SIEMENS Step 7 software to control PLC through events in their code modified at runtime

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fromcontroleventsruntimesiemenscodemodifiedgetstepsoftware

Problem

I'm a first traveler coding in WinApi, Hooks, COM objects. Then, I made a dll to inject (EasyHook) this in the target SIEMENS Step7 process and modify the software code at runtime:

Maybe I want some opinion about this kind of practices, hooking, injection, file system driver, kernel hook, modify .exe vtable before init, interprocess-communication, etc.

This code gets the DataSource from a Grid GUI(COM Object) when the user do a determined "Force" action through menu.

```
using EasyHook;
using Octtos.S7ussapx.Injected.COM.Interfaces;
using Octtos.S7ussapx.Injected.Step7.COM.Interfaces;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Runtime.InteropServices;
using System.Text;
using System.Threading;

namespace Octtos.S7ussapx.Injected
{
public class Main : IEntryPoint
{
private ClientCallbackHandler _callbackHandler;

[UnmanagedFunctionPointer(CallingConvention.StdCall, CharSet = CharSet.Unicode, SetLastError = true)]
unsafe delegate int IDispatchInvokeDelegate(IntPtr pthis, int dispIdMember, ref Guid riid, uint lcid, ushort wFlags,
ref System.Runtime.InteropServices.ComTypes.DISPPARAMS pDispParams, void* ppvResult,
ref System.Runtime.InteropServices.ComTypes.EXCEPINFO pExcepInfo, IntPtr[] pArgErr);

#region CoGetClassObject ole32.dll

private LocalHook _coGetClassObjectHook;

private LocalHook _S7StatusOCXIClassFacttoryCreateInstanceHook;
private LocalHook _S7VarServer50IClassFacttoryCreateInstanceHook2;

private bool _s7StatusOCXCOMHookCreated = false;
private bool _s7VarServer50COMHookCreated = false;

private IntPtr _S7StatusOCXIClassFacttoryCreateInstanceMethodPtr;
private IntPtr _S7VarServer50IClassFacttoryCreateInstanceMethodPtr;

private delegate uint CoGetClassObjectDelegate(ref Guid rclsid, int dwClsContext, IntPtr pServerInfo, ref Guid riid, out IntPtr ppv);

[UnmanagedFunctionPointer(C

Solution

A couple if minors I've spotted so far:

-
You should check the return value of Marshal.QueryInterface. The documentation doesn't state it explicitly but I suspect the out ppv will result in ppv being set to null if the call fails. Any subsequent code accessing that variable could therefore result in a NullReferenceException if the call failed.

-
The documentation of Marshal.QueryInterface explicitly states that you should call Marshal.Release to release the obtained pointer (decrement the COM reference count). I suspect you might be leaking COM objects by not doing so.

-
You can assign a value to a dictionary directly without checking if it exists first. So this:

if (me._s7StatusOCXDispatchInstances.ContainsKey(ppv3))
{
    me._s7StatusOCXDispatchInstances[ppv3] = me._currentInstance;
}
else { 
    me._s7StatusOCXDispatchInstances.Add(ppv3, me._currentInstance);
}


Can be replaced by this:

me._s7StatusOCXDispatchInstances[ppv3] = me._currentInstance;


-
In the Run method you have a try { } catch {} block which does nothing but simply re-throw the exception. Did you mean to perform some logging there (maybe in form of a debug or diagnostics trace)?

-
It's not clear exactly why you do this:

var bytes = Encoding.Unicode.GetBytes(varList);
var str = Encoding.Default.GetString(bytes);


varList is already a string - why this conversion through bytes? If there is a particular reason for doing this then a comment should be added explaining the why.

-
You have three blocks of code which all follow this pattern:

var methodVTableIdx = Marshal.GetComSlotForMethodInfo(method);

            int** comVTable = *(int***)ppv.ToPointer();
            int* hMethod = comVTable[methodVTableIdx];
            me._S7VarServer50IClassFacttoryCreateInstanceMethodPtr = new IntPtr(hMethod);

            me._S7VarServer50IClassFacttoryCreateInstanceHook2 = LocalHook.Create(me._S7VarServer50IClassFacttoryCreateInstanceMethodPtr, new IClassFactoryCreateInstanceDelegate(S7VarServer50CreateInstance_Hook), me);
            me._S7VarServer50IClassFacttoryCreateInstanceHook2.ThreadACL.SetExclusiveACL(new int[0]);


I think it's possible to extract this into a common method with some parameters.

Code Snippets

if (me._s7StatusOCXDispatchInstances.ContainsKey(ppv3))
{
    me._s7StatusOCXDispatchInstances[ppv3] = me._currentInstance;
}
else { 
    me._s7StatusOCXDispatchInstances.Add(ppv3, me._currentInstance);
}
me._s7StatusOCXDispatchInstances[ppv3] = me._currentInstance;
var bytes = Encoding.Unicode.GetBytes(varList);
var str = Encoding.Default.GetString(bytes);
var methodVTableIdx = Marshal.GetComSlotForMethodInfo(method);

            int** comVTable = *(int***)ppv.ToPointer();
            int* hMethod = comVTable[methodVTableIdx];
            me._S7VarServer50IClassFacttoryCreateInstanceMethodPtr = new IntPtr(hMethod);

            me._S7VarServer50IClassFacttoryCreateInstanceHook2 = LocalHook.Create(me._S7VarServer50IClassFacttoryCreateInstanceMethodPtr, new IClassFactoryCreateInstanceDelegate(S7VarServer50CreateInstance_Hook), me);
            me._S7VarServer50IClassFacttoryCreateInstanceHook2.ThreadACL.SetExclusiveACL(new int[0]);

Context

StackExchange Code Review Q#71202, answer score: 3

Revisions (0)

No revisions yet.