Class Design Advice?

[lochnessmonster]

I was wanting to try a neat little project while i was bored at work and came up with a CodeInjection class. I understand this class is VERY POORLY DESIGNED. My question is how can i fix this?? what can i do differently to make this class not so tightly coupled with the windows api functions? -thx any advice is greatly appreciated cheers

Class CodeInjection {
     private:
          HANDLE pHandle;
          Byte *preservedBytes;
          DWORD *link;
          DWORD *addressOfCave;
          void preserveBytes();
          void restoreBytes();
          
          static void CodeCave();
     public:
          void createCave();
          void destroyCave();
          void createLink();
          void destroyLink();
          CodeInjection( addressToPlaceCall, #OfBytesToOverwrite );
          ~CodeInjection();
};
 
Label1:
static void CodeInjection::CodeCave()
{
      
}
Label2:
 
 
 
 
CodeInjection::CodeInjection( BYTE *addressToPlaceCall, #ofBytesToOverwrite )
{
     link = addressToPlaceCall;
     preservedBytes = new BYTE[#ofBytesToOverwrite];
     preserveBytes();
     if( OPENPROCESSHANDLEHERE() )
     throw ERROR;
}
 
void CodeInjection::createCave()
{
     if ( ( addressOfCave = virtualAllocEx( pHandle, 0, Label2-Label1, MEM_COMMIT, PAGE_EXECUTE_READWRITE ) == NULL )
     throw   ERROR;
 
     if( ( writeprocessMemory( pHandle, addressOfCave, Label1, Label2-Label1, 0 ) == NULL )
     throw ERROR;
}
 
void CodeInjection::createLink()
{
     if( writeproccessmemory( pHandle, link, linkbuffer, sizeof(linkbuffer), 0 ) == NULL )
     throw ERROR;
 
}
 
void CodeInjection::destroyLink()
{
     if( writeproccessmemory( pHandle, link, preservedBytes, sizeof(preservedBytes), 0 ) == NULL )
     throw ERROR;
}

void CodeInject::preserveBytes()
{
     if( readprocessmemory( pHandle, addressToPlaceCall, preservedBytes, sizeof(preservedBytes), 0 ) == NULL )
     throw ERROR;
}

[mike_2000_17]

Isn't code injection malevolent? I hope you are not one of those despicable hackers.

>>I understand this class is VERY POORLY DESIGNED.
It's also very incomplete. You are not initializing most of your data members.

>>My question is how can i fix this??
The main problem that I see is that you are allocating resources and performing operations while throwing a bunch of meaningless exceptions. First, you should come up with a set of exception classes that make sense, throwing one generic ERROR exception is just useless, you're not really giving any information to the user of your class as to why there was a failure. Why report an error if you give no information about it?

Second, you need a destructor! Maybe you omitted to post it, but that would be quite a huge omission. When you allocate resources and perform read-write operations in a "preserve the original" fashion, and you are throwing exceptions around, you have to make sure that if an exception is thrown, the state is not left invalid after stack unwinding. That's why RAII is so useful in C++. With RAII, you do the initialization in the constructor (basically, the operations that can fail), and so, if the operations fail, you throw an exception and then the destructor will be called to undo what the constructor did, and at the end, you can make sure that the state of the system is made valid again by the destructor. For example, with createCave(), if the allocation succeeds but the writing fails, you don't want to leave the allocated virtual memory in place, you want to deallocate it. If you create a class called say "CodeVirtualMemory" that upon construction will allocate memory and then has a function to write some chunk of memory. With this, you can use a create-and-swap idiom:

class CodeInjection {
  private:
    HANDLE pHandle;
    CodeVirtualMemory mCave; //assume you have some data member for the cave memory.
  //...

void CodeInjection::createCave() {
  //create temporary CodeVirtualMemory object to allocate virtual memory:
  CodeVirtualMemory temp(pHandle, Label2-Label1);     //give it the necessary info.
  //here, if the construction failed, the destructor will clean up
  //if the construction succeeded, we can proceed:
  temp.write(Label1,Label2-Label1); //write the code memory.
  //here, if the writing failed, the stack will unwind and clean-up with the destruction of "temp".
  //finally, if all went well, swap with the data member:
  mCave.swap(temp); //transfers content of temp to mCave and leaves temp to destroy whatever was in mCave.
};

You see that the above is exception-safe, always keeps the object in a valid state, and will not under any exceptional circumstance leave crap behind.

Finally, I doubt that you need all those separate "void function(void)". Whenever you have a situation where the normal use of your class is to create it, then call a sequence of void() functions and then, when finished, call a reverse sequence of void() functions and destroy the object. Typically, in these cases, a simple constructor and destructor will do just fine, why force the user to execute this rigid sequence of calls when you can do it in the constructor and ensure orderly destruction via the destructor.

>>what can i do differently to make this class not so tightly coupled with the windows api functions?

There are two things you can do to help with that. First, because of what I mentioned above, you can observe that a better design would probably involve a few RAII classes to help this CodeInjector class. I bet you could just wrap all the platform-specific code in these helper classes. Then, you just implement a win32 version in one folder, a linux version in another, etc. etc., and use standard compiler flags to choose which header to include. If those headers include classes of the same name, you won't have to change anything to your CodeInjector class. Alternatively, you can also use compiler flags in the single implementation of CodeInjector or its helper classes to put the different platform-specific API calls.

The second thing you can do is a Cheshire Cat (or PImpl). You can forward-declare an implementation class, hold a pointer to it as data member to CodeInjector, and then define that class in the cpp file only (then you have one cpp file per platform, but all the headers remain the same). For example:

class CodeInjector_impl; //forward-declaration.

class CodeInjector {
  private:
    CodeInjector_impl* pImpl; //Pointer to the implementation object.
    //basically, no other data members are needed, they are all in pImpl.
  public:
    //public interface, as before.
};

//in the CPP file:
#include "CodeInjector.h"

class CodeInjector_impl {
  //write all the platform-specific code you like here.
};

CodeInjector::CodeInjector(/*..params..*/) {
  pImpl = new CodeInjector_impl(/*..params..*/);
};

CodeInjector::~CodeInjector() {
  delete pImpl;
};