0

In a DLL compiled from C++ with MS Visual Studio 2003, I'm generating a file path by getting a string from SHGetFolderPath and appending a file name, then trying to open that file for output.

I have some globals, one function to setup the file path, and another function to open the file, all shown below.
The Logger class declaration isn't shown, but it has a member declared as ofstream *ofs which is the file object created in Logger's constructor.

In constructorBase(), if I call initLogFN() to setup the file path, ofs->open fails.
However, if I comment out the initLogFN() call and uncomment the line that sets logPath to a hard-coded string literal, ofs->open succeeds.

When debugging in either case, Visual Studio correctly shows logPath as having the value "C:\Users\sleis\AppData\Roaming\Address-IT.log" before ofs->open is called.
However in the failing case, after the ofs->open call, logPath's value changes to six characters that seem random but are the same every time I run my test app.

Why do the two cases behave differently, and how can I fix the failing case?


#if defined (WIN32)
const char dirSep[]="\\";
#else
const char dirSep[]="/";
#endif

const char logFN[]="Address-IT.log";
char *logPath=0;

bool initLogFN()
{
    if (logPath!=0) return false;

#if defined (_DEBUG)
#if defined (WIN32)
    char path[MAX_PATH];
    HRESULT result=SHGetFolderPath(NULL,CSIDL_APPDATA,NULL,SHGFP_TYPE_CURRENT,path);
    if (result!=S_OK) return false;
    strcat(path,dirSep);
    strcat(path,logFN);
    logPath=path;
    return true;
#endif
#endif

    return false;
}

void Logger::constructorBase()
{
    if (!initLogFN()) return;
    //logPath="C:\\Users\\sleis\\AppData\\Roaming\\Address-IT.log";

    ofs->open(logPath,ios_base::out | ios_base::app);

    if ((ofs->rdstate() & std::ifstream::failbit)!=0) {
        std::cout << std::endl << "Error opening log file.";
    } else if (ofs->is_open()) {
        std::cout << std::endl << "Log file opened successfully.";
    }
}
0

3 Answers 3

2

The array path is an automatic variable local to initLogFN. When you set logPath=path, it causes the logPath variable to point into this array. But when the function returns, that array goes out of scope, which means whatever memory it occupied may be subsequently overwritten. Now path points to garbage.

Never return a pointer to a local variable.

You essentially have three options for returning a C-style string from a function:

  1. Have the function dynamically allocate the memory for the string, using malloc or (in C++) new. If you do this, then the caller will have to eventually call free or delete to avoid memory leaks.

  2. Pass the char* as a parameter to the function. The caller should pre-allocate the memory for the string and then pass the pointer into the function. The function writes into the location the pointer points to.

  3. Use a global variable, and return a pointer to that.

Of these approaches, #2 is the most common. The C and C++ standard libraries take this approach (e.g. fgets), and generally require another parameter indicating the size of the buffer passed in, so the function knows not to write past the end of allocated memory.

The disadvantage of approach #3 is that it is not reentrant. A few old Unix library functions take approach #3 (e.g. ttyname), but reentrant versions that take approach #2 were later introduced (e.g., ttyname_r).

1
  • This answer seems clearest to me, but I also gave the others up-votes. The code is working now, so thanks.
    – Emma Leis
    Jan 28, 2014 at 4:56
2

The problem is that you are trying to reference a variable (char path[MAX_PATH]) which no longer exists.

First you create this variable in initLogFN. Then you assign logPath to point at the address of it. But outside the scope of initLogFN (aka after it returns), the memory logPath is now pointing at is complete garbage.

The reason it still looks okay in the debugger BEFORE you call the next function is because it exists on the stack. However, the stack is overwritten with the call to the next function. And your string is filled with garbage. The reason it's filled with the same six characters of garbage each time? Let's call that luck.

There are a few ways you could fix this problem.

  1. You could dynamically allocate the character array, for example by saying char* path = new char[MAX_PATH]. If you do this, make sure you clean it up after you're done by saying delete path.
  2. You could declare path within your constructor base, and then pass it into your initLogFN. For example, your new initLogFN would have this signature bool initLogFN(char* path), and you would call it like this (from Logger::constructorBase) char path[MAX_PATH]; initLogFN(path);
  3. The third way would be to remove this seemingly unnecessary helper function (initLogFN) and just put that code into the constructorBase function.

The choice is yours and yours alone! Good luck! Just remember, if you don't dynamically allocate something (using new or malloc), then don't try to store a global pointer to it, and definitely don't try to reference it outside of the scope in which it was declared!

2

The statement logPath=path; sets the pointer logPath to point to the same location as path. As soon as the function exits, the memory for path (and thus the memory for logPath) is deallocated and the memory gets reallocated and overwritten by further function calls.

You need to allocate data for logPath and copy with strcpy the data from path to logpath to prevent that.

Not the answer you're looking for? Browse other questions tagged or ask your own question.