2

Possible Duplicate:
Pointer to local variable

I've read a lot of other topics on this site about the same problem knowing it would be common. But I guess I'm dumb and can't figure out the proper way to do it. So, I apologize for yet another one of these questions and I'm hoping someone can give me a simple solution and/or explanation.

Here's the entire code:

Main.c

#define WIN32_LEAN_AND_MEAN

#include <Windows.h>
#include <stdlib.h>
#include <tchar.h>


LPTSTR GetApplicationPath ( HINSTANCE Instance );


int APIENTRY _tWinMain ( HINSTANCE Instance, HINSTANCE PreviousInstance, LPTSTR CommandLine, int Show )
{
    LPTSTR sMessage = GetApplicationPath ( Instance );

    MessageBox (
        NULL,
        sMessage,
        _T ( "Caption!" ),
        MB_OK
    );

    return 0;
}


LPTSTR GetApplicationPath ( HINSTANCE Instance )
{
    _TCHAR sReturn[MAX_PATH];

    GetModuleFileName ( (HMODULE) Instance, sReturn, MAX_PATH );

    return sReturn;
}
Community
  • 1
  • 1
guitar-
  • 1,051
  • 5
  • 15
  • 21

4 Answers4

9

Right now, you're returning the address of an automatic (stack) array. This is always wrong, because as soon as the function ends, so does that memory's lifetime.

You need to use malloc (and free), or other dynamic allocation. E.g.:

_TCHAR *sReturn = malloc(sizeof(_TCHAR) * MAX_PATH);

I've omitted error checking. Then later, the calling code should free it. After the MessageBox in _tWinMain:

free(sMessage);
Matthew Flaschen
  • 278,309
  • 50
  • 514
  • 539
3

Change 

    _TCHAR sReturn[MAX_PATH];

in your "GetApplicationPath" to

    static _TCHAR sReturn[MAX_PATH];

If only one thread is going to be calling GetApplicationPath(), dynamically allocating this string is overkill. Marking a local variable as static allocates space for it with the globals, so that space doesn't get stepped on when you exit from the function.

mjfgates
  • 3,351
  • 1
  • 18
  • 15
  • It doesn't really make sense to do this unless you were optimizing for both stack and heap use. Generally nobody bothers to optimize stack usage, and it's safer to not presume the threading situtation. – Matt Joiner Sep 18 '10 at 05:55
2

Consider modifying:

  LPTSTR GetApplicationPath ( HINSTANCE Instance )

to something like

  void GetApplicationPath ( HINSTANCE Instance, LPTSTR str )

This will eliminate the need to return a local variable.

skimobear
  • 1,188
  • 10
  • 12
  • You could do it this way, but `GetApplicationPath` should just take a `LPTSTR`, since you're just writing chars to that location, not setting the caller's variable. Also, it must take a length parameter or document that it must be `MAX_PATH`. – Matthew Flaschen Sep 18 '10 at 02:41
  • @Matthew - I fixed the pointer issue you mentioned. Thx. I was thinking I would leave the MAX_PATH issue you mentioned to the imagination of the user. The argument really could be what ever they prefer (ex. char pathname[MAX_PATH]). Cheers – skimobear Sep 18 '10 at 02:57
1

The warning says it all. In the function GetApplicationPath you are returning sReturn which is local to the function. Local variables cease to exist once the function returns and reference to them after function returns is incorrect.

To overcome this you can dynamically allocate space sof sReturn as:

sReturn = malloc(sizeof(_TCHAR) * MAX_PATH);
codaddict
  • 445,704
  • 82
  • 492
  • 529
  • 3
    In C, the return of `malloc` should not be casted. When forgetting to include "stdlib.h" this may loose bits of your address on 64 archs... Casting `malloc` is a C++ habit. There you need to do it, but you should usually use `new`, anyhow. – Jens Gustedt Sep 18 '10 at 06:09