Good coding practice?

[Zach1188]

This is my first C++ program that I put any serious effort into. You may have seen the trick that allows you to start explorer.exe as the SYSTEM user (by running "at /interactive xx:xx explorer.exe", adding 1 minute to the current time). It takes a lot of jumping through loops, and I had nothing better to do, so I made a program that fetches the current time, adds one minute, and runs the command. I went a step further, and made it start on startup, so that one could always log in as SYSTEM if they wanted to.

However, the program forces itself to run instead of userinit.exe, afterward running userinit.exe manually using at.exe, effectively starting explorer.exe as SYSTEM. So, I made a quick .bat uninstaller to restore the registry afterward. And because of the registry modification, it will appear as a virus by most antivirus software.

Anyway, enough rambling. Basically what I'm asking is, since this is more or less my first program, did I use good coding practice? Are there any obvious ways I could improve my program?

Edit: Actually, after posting I realized something. For some reason, the time GetSystemTime() gave me was 7 hours ahead, so I was forced to take 7 hours off the current time. I don't know why it didn't click with me, but I guess GetSystemTime() gets GMT 0, and I am in mountain time (GMT -7). So basically, this program will work great for anyone in mountain time XD. So how can I get the systems timezone, in 0, -7, 5 etc. format?

Disregard the above, I just need to use GetLocalTime instead.

#define WINVER 0x0500
#include <windows.h>
#include <string>
#include <sstream>
#include <direct.h>
#include <fstream>
    
SYSTEMTIME st;
std::string exePath, drive;

std::string calcTime(int tMinAdd) {
    std::ostringstream os, hr, mn;
    int iHr, iMn;
    int tRemainder;

    GetLocalTime(&st);
    hr << st.wHour;
    mn << st.wMinute;
    
    std::istringstream sh(hr.str());
    sh >> iHr;
    std::istringstream sm(mn.str());
    sm >> iMn;
    
    tRemainder = 60 - iMn;
    iMn = iMn + tMinAdd;
    if(iHr < 0)
        iHr = 24 - (iHr * -1);
    if(iMn > 59) {
        iMn = iMn - (tRemainder);
        iHr++;
        if(iHr > 23)
            iHr = iHr - 24; 
            
    }
    //MessageBox(NULL, os.str(), "Time after added", MB_OK);
    os << iHr << ":" << iMn;
    return(os.str());
}

std::string makeUnin(std::string UninstBatPath) {
    std::ofstream UninstBat;
    
    if(UninstBatPath == "")
        UninstBatPath = drive + ":\\Program Files\\sysLogin\\uninst.bat";
    
    UninstBat.open(UninstBatPath.c_str());
    UninstBat << "@echo off" << std::endl;
    UninstBat << "echo Creating .reg file...." << std::endl;
    UninstBat << "pause" << std::endl;
    UninstBat << "echo REGEDIT4 > uninst.reg" << std::endl;
    UninstBat << "echo [HKEY_LOCAL_MACHINE\\Software\\Microsoft\\Windows NT\\CurrentVersion\\Winlogon] >> uninst.reg" << std::endl;
    UninstBat << "echo \"Userinit\"=\"" << drive << ":\\\\Windows\\\\system32\\\\userinit.exe, \" >> uninst.reg" << std::endl;
    UninstBat << "echo [HKEY_LOCAL_MACHINE\\SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\RunOnce] >> uninst.reg" << std::endl;
    UninstBat << "echo \"DeleteRemaining\"=\"cmd /C rmdir /s /q \\\"" << drive << ":\\\\Program Files\\\\sysLogin\\\"\" >> uninst.reg" << std::endl;
    UninstBat << "echo _" << std::endl;
    
    UninstBat << "echo Done. Restoring registry now..." << std::endl;
    UninstBat << "pause" << std::endl;
    UninstBat << "regedit /s uninst.reg" << std::endl;
    UninstBat << "echo _" << std::endl;
    UninstBat << "echo You must restart for changes to take effect, all sysLogger files will then be deleted. ";
    UninstBat << "If you installed ";
    UninstBat << "sysLogin but never rebooted or logged in as SYSTEM, a reboot may not be neccesary. " << std::endl;
    UninstBat << "set /p choice=Would you like to reboot now? (y, n)" << std::endl;
    UninstBat << "if '%choice%'=='y' goto reboot" << std::endl;
    UninstBat << "if '%choice%'=='Y' goto reboot" << std::endl;
    UninstBat << "exit" << std::endl;
    UninstBat << ":reboot" << std::endl;
    UninstBat << "shutdown -r -t 0" << std::endl;
    return ("\"" + UninstBatPath + "\"");
}
    
int install() {
    std::string exeDir;
    std::ifstream exe;

    exePath = drive + ":\\Program Files\\sysLogin\\sysLog.exe";
    exe.open(exePath.c_str(), std::ios::binary | std::ios::in);
    
    if(!exe.is_open()) {
        exe.close();
        if(MessageBox(NULL, "Would you like to install sysLogger now?", "Installing....", MB_YESNO | MB_ICONQUESTION) == IDNO)
            return(2);
        char self[MAX_PATH];
        char startup[MAX_PATH] = "";
        HMODULE GetModH = GetModuleHandle(NULL);
        makeUnin("");
    
        exeDir = drive + ":\\Program Files\\sysLogin";
        CreateDirectory(exeDir.c_str(), NULL);
        
        GetModuleFileName(GetModH, self, sizeof(self));
        CopyFile(self, exePath.c_str(), true);
        
        HKEY hKey;
        strcpy(startup, exePath.c_str());
        RegOpenKeyEx(HKEY_LOCAL_MACHINE, "Software\\Microsoft\\Windows NT\\CurrentVersion\\Winlogon", 0, KEY_SET_VALUE, &hKey);
        RegSetValueEx(hKey, "Userinit", 0, REG_SZ,(const unsigned char*)startup, sizeof(startup));
        RegCloseKey(hKey);
        makeUnin("");
        exe.open(exePath.c_str(), std::ios::binary | std::ios::in);
        if(exe.is_open()) {
            exe.close();
            return(1);
        } else if(!exe.is_open()) {
            exe.close();
            return(-1);
        }
    }
    exe.close();
    return(0);
}

int main() {
    drive = _getdrive() + 0x40;
    int result = install();
    if(result == 1) {
        MessageBox(NULL, "SysLogger was successfully installed. Reboot to automatically login as SYSTEM. To uninstall, run %:\\Program Files\\sysLogin\\uninst.bat.", "Success", MB_OK | MB_ICONINFORMATION);
        return 0;
    } else if(result == -1) {
        int response = MessageBox(NULL, "SysLogger failed to install. Run cleanup?", "Error", MB_CANCELTRYCONTINUE | MB_ICONEXCLAMATION);
        if(response == IDTRYAGAIN) {
            main();
            return 0;
        } else if(response == IDCONTINUE) {
            system(makeUnin("cleanup.bat").c_str());
            ShellExecute(NULL, "open", "cmd.exe", "/K del cleanup.bat", NULL, 0);
            ShellExecute(NULL, "open", "cmd.exe", "/C del uninst.reg", NULL, 0);
            return 0;
        } else if(response == IDCANCEL)
            return 0;
    } else if(result == 2)
        return 0;
    
    std::string cmd = calcTime(1) + " /interactive \"" + drive + ":\\Windows\\system32\\userinit.exe\"";
    drive = drive + ":\\";
    ShellExecute(NULL, "open", "at", cmd.c_str(), drive.c_str(), 0);
    
    //MessageBox(NULL, calcTime(1).c_str(), "Installing....", MB_OK);
    return 0;
}

[StuXYZ]

Hi

Well, I am not going to comment on the purpose of the code. It does make me very very glad I don't use windows for anything (including work).

So things that I would pick you up on if you worked for me. .

The first problem is for a c++ example there are no user defined classes. That really shows were the problem lie in understanding.

Second : Comments!!!!! -- What this problem is doing functionally is not documented. Yes you wrote it for yourself, but please consider what happens in ten years time.

Third: Too much work is done in main, that would be better in a function. Along with global variable that really don't need to be.

Forth: Way to little use of clear constants. Particularly directory names etc could be in const string block at the top, or in a namespace. (Just one place to look for it)

Fifth: strcpy(startup, exePath.c_str()); why bother, you only use this once and then you could have put
exePath.c_str() ?? [If you HAD to have an array of MAX_PATH then add a comment. ]

Sixth: Very inconsistent use of lines like d=d+"ab"; and d+= Seventh: Consider

else if(!exe.is_open()) 
{
    exe.close();  // how do you close something that is not open?
    return(-1);
}

well that is a summary of things that quickly come to mind.
I STRONGLY recommend that you use a much more open spacing for your code ({ / } on a new line etc). This is minor, since we all use code layout managers to get our own preferences BUT it helps you read your own code.

You have not used using namepace std; . Well done. I believe that will save you 1000's of hours of debugging in the long run.

Your have almost no const variables. They are there MAINLY to make the compiler enforce your intent. Help the compiler help you.

You pass string by copy value, not by reference.

Checks... what happens if files/directories don't exist?

Overall it is not terrible. If that is your first serious program congratulations. But your next one will be much much better. After a couple of programs you will start to develop a style, then you are really getting somewhere.

Have a read of the linux kernel coding standard
http://www.yolinux.com/TUTORIALS/LinuxTutorialC++CodingStyle.html
and the book
C++ Coding StandardsL 101 Rules, Guidelines and Best Practices
by Herb Sutter and Adnrei Alexandrescu

[ddanbe]

and I had nothing better to do

IMHO you could improve your program by deleting it and make something that's really usefull.

[StuXYZ]

In comment to ddanbe:

Yes I fully agree BUT I can (just about) remember learning 6502 assembler because I too wanted (a) crack the Elite (a game) protection system [they locked each block on the tape!] (b) to do the split mode screen display. In that process I learnt about indirect references and lots of stuff that made C an easy language to learn.

So who am I to say that your code is worthless.

N.B. 6502 is a 1Mhz 8 bit processor with 3 registers.

[ddanbe]

Hah those days!
Apple II , Commodore PET what a delight!

Code IS worthless(IMHO) if it just even tries to serve malicious purposes. You could also learn assembler without cracking some whatever...

[Murtan]

I still remember some of those days...

I started out trying to modify a "starwars" game (shoot the tie fighter) for the Commodore PET. I found that setting the high bit of the display byte would cause the character to be inverted (black on white vs white on black). So I thought hey, I could make the screen 'flash' when the ship blew up. So I wrote it up in BASIC to peek() and poke() all of the screen memory. I was SADLY disappointed in the performance, you could see the code work through the screen.

I'm not sure where I got the idea from now (its been a little while) but I was prompted to try the idea again in machine code. I found a book on 6502 assembler at the local Radio Shack and used it to write the code (and generate the bytes too if I rember right). I ended up writing the routine to XOR the high bit, so you called the routine once to invert the screen and once again to restore it. The performance of the assembly actually required a small delay between the two calls so that it looked like more than a screen flicker. (Much more satisfying.)

@Zach1188:

What does this code do (other than convert to a string and then back)?

GetLocalTime(&st);
    hr << st.wHour;
    mn << st.wMinute;
    
    std::istringstream sh(hr.str());
    sh >> iHr;
    std::istringstream sm(mn.str());
    sm >> iMn;

This code is all just to add one minute to the time?

tRemainder = 60 - iMn;
    iMn = iMn + tMinAdd;
    if(iHr < 0)
        iHr = 24 - (iHr * -1);
    if(iMn > 59) {
        iMn = iMn - (tRemainder);
        iHr++;
        if(iHr > 23)
            iHr = iHr - 24;

According to microsoft, the 'official' way is to:

It is not recommended that you add and subtract values from the SYSTEMTIME structure to obtain relative times. Instead, you should:

• Convert the SYSTEMTIME structure to a FILETIME structure.
• Copy the resulting FILETIME structure to a ULARGE_INTEGER structure.
• Use normal 64-bit arithmetic on the ULARGE_INTEGER value.

std::string calcTime(int tMinAdd) {
	SYSTEMTIME st;
	FILETIME ft;
	ULARGE_INTEGER temp;

    GetLocalTime(&st);
	SystemTimeToFileTime(&st, &ft);
	temp.HighPart = ft.dwHighDateTime;
	temp.LowPart = ft.dwLowDateTime;
	// FILETIME Contains a 64-bit value representing the number of 100-nanosecond intervals since January 1, 1601 (UTC).
	// so * 10 to get micro seconds, * 1000 more to get milliseconds, * 1000 more to get seconds
	// or if you want to combine them all (some compilers will auto-combine constant math) * 10000000
//	temp.QuadPart += tMinAdd * 60 * 10 * 1000 * 1000;
	temp.QuadPart += tMinAdd * 600000000;
	FileTimeToSystemTime(&ft, &st);

	std::ostringstream os;
	os << st.wHour << ":" << st.wMinute;
	return os.str();
}

or, even simpler for your particular case:

std::string calcTime(int tMinAdd) {
	SYSTEMTIME st;
	GetLocalTime(&st);
	int iHr = st.wHour;
	int iMn = st.wMinute;

	iMn += tMinAdd;
	if (iMn > 59) {
		iHr = (iHr + (iMn / 60)) % 24;
		iMn = iMn % 60;
	}

	std::ostringstream os;
	os << iHr << ":" << iMn;
	return os.str();
}