Search code examples
c++stringfstream

Why does fstream.open(filename) work with literal but not with generated string?


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.";
    }
}

Solution

  • 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).