• c (unregistered)

    Love the: if it don't work - do it anyway...

     /c
     

  • Rishu73 (unregistered)

    I spotted the leak first (we are in leak detection mode, so I'm pretty hardwired to spot those first).

     The buffer overrun is more WTF though...

     

  • (cs)

    WTF??

    Today's post so early in the day!!

  • (cs)

    That's a pretty clear example of what happens when somebody doesn't really understand the problem. However, the whole conditional behaviour of many of the Win32 functions is perhaps a bigger WTF. On the one hand, I can see a need for such a thing when the possibility for buffer overrun exists, but on the other hand, shouldn't it be a different function that accesses the same environment string behind the scenes to make the behaviour a little bit sensible?

  • Iain (unregistered)

    "That book on C you almost bought at the garage sale down the block would have been a good investment."

     char* path = new char[256]

    C++, surely?

     
    Incidentally - what's wrong with just a normal textarea and BBCode? these formatting enabled text areas are just as wank as the rest of community server...

  • nuno (unregistered)

    anyone care to explain?

    it doesnt look good but i cant understand why :p 

  • Jason (unregistered) in reply to Rishu73

    Two buffer overruns.


    I especially like how, after he detects the first buffer overrun, he takes extra care to ensure that the second call is also a buffer overrun.

     

  • Sam (unregistered) in reply to nuno

    I am not a c++ guy but it looks like CString(path) is copying the data out of path and then path is getting orphaned when the function returns. The second problem is that the weird if( length >256) code by definition will cause an overrun. They should have expanded the buffer to pathLength if it was greater thatn 256.

     

  • SM (unregistered) in reply to Sam
    Anonymous:

    I am not a c++ guy but it looks like CString(path) is copying the data out of path and then path is getting orphaned when the function returns.

     In so many words, that's exactly what's happening  -- the new operator allocated memory for path.  After the function exits, the pointer to path is gone, yet path still exists in memory (but is unreachable and unusable). 
     

  • Ryan (unregistered)

    Offtopic:  Can we, the people of the internet, please have a moratorium on the "trademark and capitalization" joke?

    "Memory leaks are a Big Deal TM,"
    "It is a Good Thing TM,"  "it is a Bad Thing"

    Just stop it please,  this stopped being funny years ago.

  • (cs) in reply to Jason
    Anonymous:

    Two buffer overruns.


    I especially like how, after he detects the first buffer overrun, he takes extra care to ensure that the second call is also a buffer overrun.

     

    I'm pretty sure there is only one buffer overrun: if your buffer length (the first argument) is not big enough to hold the path, then GetTempPath returns the needed length and does nothing (or just fills the buffer up to the specified limit).

    Well there could be two buffer overruns depending on how the 0-termination is handled: I'm not sure whether the size you give to GetTempPath includes the null terminator or not... but since GetTempPath returns a size that does not include the null terminator, that's probably what it expects (the number of characters it can actually write), which means that if the path is exactly 256 chars GetTempPath won't error but will write 257 bytes in a 256b buffer.

    Probably a much rarer occurence than the first buffer overrun though.

    And in any case, that means that there's only ever one buffer overrun. Either of 1 byte or of (needed length - 256) bytes, but both can't happen at the same time (even though the second one would overrun the first anyway).

  • dividius (unregistered) in reply to ParkinT
    ParkinT:

    WTF??

    Today's post so early in the day!!

    Silly Rabbit, this is a very late YESTERDAY's CSOD.

     err.... well ok maybe it IS an early today's CSOEODOS (Code Snippet Of Every Other Day Or So).

  • Rich (unregistered) in reply to SM
    Anonymous:

    yet path still exists in memory (but is unreachable and unusable). 
     

    Nonsense. It is available for use by the buffer overrun :D

     

    Rich 

  • Jalf (unregistered)

    Well, look on the bright side. At least the buffer overrun happens on dynamically allocated memory, which means there's a (slim) chance he'll get an access violation instead of just corrupting other data as would happen if he'd done it on the stack. ;)

  • (cs) in reply to ParkinT
    ParkinT:

    WTF??

    Today's post so early in the day!!

     
    Actually it's late!  I was feeling poorly last night (I'm blaming the flu shot!) and totally forgot to post last night!

    - Tim

    http://www.ThePhpPro.com/ 

  • (cs)

    Egads.

     
    The memory leak is obvious.  Depending on what exactly GetTempPath() does, there may be 2 buffer over-runs here, or maybe even none.

    If the function is sane and only fills path up to the length specified, and then is a little odd and returns the full length of what it would have put into path if it had been given the room, then yes, we have a buffer overflow.  This does look like what happens, else wtf are we coding for pathLen > 256 in the first place?  On the off chance, however, that it returns the actual number of bytes loaded into path *up to* the limit passed in, then the if block is useless and will never be reached, and we also don't have a buffer overflow - just a memory leak and some dead code.

    Of course, if GetTempPath() was written by the same developer who wrote GetTempDirectory, then there may be even more wtfs to be found there.
     
    The CString being returned is not orphaned since it is created on the stack and will be properly destructed as it goes out of scope.  A copy will be returned to the calling function, which will presumably be set up properly by CString's copy constructor.

    ... but the real WTF is still the forum software. 

  • (cs)

    GetTempPath is a standard win32 platform sdk function from kernel32. If the path can't fit in the buffer, it returns the number of bytes required to store the path... If it fits, then the size returned doesn't include the null terminator. Could it be more confusing? The definition for GetTempPath calls the first parameter the size of the buffer, which leads me to believe it's the overall size including a null terminator. lol... this function is so funny though lol...

  • (cs) in reply to Iain
    Anonymous:

    "That book on C you almost bought at the garage sale down the block would have been a good investment."

     char* path = new char[256]

    C++, surely?

     
    Incidentally - what's wrong with just a normal textarea and BBCode? these formatting enabled text areas are just as wank as the rest of community server...

    I write C++ code every day and yet I still learn some things occasionally from that C book I did pick up at a garage sale. :) 

  • Shrek (unregistered)

    The real WTF is people still use languages where they have to give a crap about pointers, buffer overruns and orphaned memory.

     

     

  • anon the cowardly (unregistered)

    I'm curious about the GetTempPath function. So the contract that the function has essentially says "You have to give me a char array and tell me how long it is, then I'll return a number that tells you if you guessed the right length or not." I think I discovered another WTF.

  • Rich (unregistered) in reply to wk2x

    A real wtf is that people are speculating about how gettemppath works when it is a google and click away...

     

     

    nBufferLength
    [in] Size of the string buffer identified by lpBuffer, in TCHARs.
    lpBuffer
    [out] Pointer to a string buffer that receives the null-terminated string specifying the temporary file path. The returned string ends with a backslash, for example, C:\TEMP\.

    Return Values

    If the function succeeds, the return value is the length, in TCHARs, of the string copied to lpBuffer, not including the terminating null character. If the return value is greater than nBufferLength, the return value is the length, in TCHARs, of the buffer required to hold the path.

     

     

    So yes, calling it with the value of 256 is correct.

     

    Rich 

  • Anonymous (unregistered) in reply to Shrek
    Anonymous:

    The real WTF is people still use languages where they have to give a crap about pointers, buffer overruns and orphaned memory.


     

    Pointers rule!
    Don't complain about it because you are too lazy to use them properly.  Yes, there are issues you have to deal with when using pointers that can make them a pretty big pain when you don't know what you are doing, but they are extremely powerful and covering your *** from the main problems is pretty easy.

  • RandomLurker (unregistered)

    From WINDEF.h


    #define MAX_PATH          260


    Damn, he was only 4 bytes short.

  • Hexar (unregistered) in reply to Rich

    I'm surprised no one has even tried to fix the code yet.  Here's my attempt; bear in mind I haven't tested this, but feel free to jump down my throat for errors anyway:

    CString GetTempDirectory()
{
    char* path = new char[256];
    int pathLen  = GetTempPath(255, path);
    if (pathLen > 255)
    {
        delete[] path;
        path = new char[pathLen+1];
        GetTempPath(pathLen, path);
    }
    path[pathLen] = 0;

    CString s(path);

delete[] path;

return s;

    

    }

    This code
    (a) Fixes the missing delete problem
    (b) Correctly adds the null terminator to the string and
    (c) Re-allocates memory correctly when the path is too long for the buffer.

    Unless, of course, I made a glaring error.

  • Larry Laffer (unregistered) in reply to GoatCheez 
    So a working implementation should be: 
    CString GetTempDirectory()
    {
    	int pathLen  = GetTempPath(0, NULL);
    	char* path = new char[pathLen + 1];
            GetTempPath(pathLen, path);
    	CString cpath(path);
    	delete[] path;
    	return cpath;
    }
     
    Sycophants of the world unite! Because you're such nice people, with lovely personalities, and great taste in clothes.
  • Bill (unregistered) in reply to Rich
    Anonymous:

    A real wtf is that people are speculating about how gettemppath works when it is a google and click away...

     

     

    nBufferLength
    [in] Size of the string buffer identified by lpBuffer, in TCHARs.
    lpBuffer
    [out] Pointer to a string buffer that receives the null-terminated string specifying the temporary file path. The returned string ends with a backslash, for example, C:\TEMP\.

    Return Values

    If the function succeeds, the return value is the length, in TCHARs, of the string copied to lpBuffer, not including the terminating null character. If the return value is greater than nBufferLength, the return value is the length, in TCHARs, of the buffer required to hold the path.

     

     

    So yes, calling it with the value of 256 is correct.

     

    Rich 

     

    Well, that makes two official buffer overflows.  Since on MANY windows platforms tchar ends up being two bytes, the first call will overflow by up to 256 bytes.

    Then of course there is the classic buffer overflow where the caller didn't follow the standard Microsoft Pattern of reallocating the buffer if it the origional call fails to the correct size.

     To me, the memory leak is a minor problem, easily detectable with the use of any decent memory manager - another WTF is why most coders don't use such technologies

  • Bill (unregistered) in reply to Larry Laffer
    Anonymous:
    So a working implementation should be: 
    CString GetTempDirectory()
    {
    	int pathLen  = GetTempPath(0, NULL);
    	char* path = new char[pathLen + 1];
            GetTempPath(pathLen, path);
    	CString cpath(path);
    	delete[] path;
    	return cpath;
    }
     
    Sycophants of the world unite! Because you're such nice people, with lovely personalities, and great taste in clothes.

     So close - give you most credit.  If you change from a char * path to a tchar * (that covers the size of the character depending on your platform) you win... Currently you will Guarantee a buffer overflow.

     Thank you for playing, come again

    Just noticing that my captcha is "Clueless"

  • Bill (unregistered) in reply to Larry Laffer
    Anonymous:
    So a working implementation should be: 
    CString GetTempDirectory()
    {
    	int pathLen  = GetTempPath(0, NULL);
    	char* path = new char[pathLen + 1];
            GetTempPath(pathLen, path);
    	CString cpath(path);
    	delete[] path;
    	return cpath;
    }
     
    Sycophants of the world unite! Because you're such nice people, with lovely personalities, and great taste in clothes.

     So close - give you most credit.  If you change from a char * path to a tchar * (that covers the size of the character depending on your platform) you win... Currently you will Guarantee a buffer overflow.

     Thank you for playing, come again

    Just noticing that my captcha is "Clueless"

  • (cs)

    The Real WTF (TM)* is that you need to write code like this (with pointers and stuff, not necessarily with WTFs) to get information out of the OS in the first place.

    <font size="1">*: Yes, I did just write this because we were all asked to stop it. Sue me, I'm a contrarian.</font>

  • PseudoNoise (unregistered) in reply to Larry Laffer

    Almost how I'd do it, Larry Laffer, except you're copying memory.

    CString GetTempDirectory()

    {
    	int pathLen  = GetTempPath(0, NULL);
    	CString retval;
    	char *pStr = retval.GetBufferSetLength (pathlen+1);
            GetTempPath(pathLen, pStr);
    	retval.ReleaseBuffer ();
    	return retval;
    }

    Embedded programmers of the world unite! (Sorry I can't figure out code formatting)

  • anon the cowardly (unregistered) in reply to Larry Laffer

    In the first line of the body of this function you got at the heart of my earlier point with quite admirable sarcasm. You don't see a whole lot of people that are able to write poetry in code like that. Cudos. Anyway, with the GetTempPath you don't know how big to make your array. You could use MAX_PATH + 1, but that's annoying when it is usually just "C:\temp" characters long.  

    Anonymous:

    So a working implementation should be: 
    CString GetTempDirectory()
    {
    	int pathLen  = GetTempPath(0, NULL);
    	char* path = new char[pathLen + 1];
            GetTempPath(pathLen, path);
    	CString cpath(path);
    	delete[] path;
    	return cpath;
    }
     
    Sycophants of the world unite! Because you're such nice people, with lovely personalities, and great taste in clothes.
  • PseudoNoise (unregistered) in reply to PseudoNoise
    Anonymous:

    Almost how I'd do it, Larry Laffer, except you're copying memory.

    CString GetTempDirectory()

    {
    	int pathLen  = GetTempPath(0, NULL);
    	CString retval;
    	char *pStr = retval.GetBufferSetLength (pathlen+1);
            GetTempPath(pathLen, pStr);
    	retval.ReleaseBuffer ();
    	return retval;
    }

    Embedded programmers of the world unite! (Sorry I can't figure out code formatting)

     

    Ah crud, just realized I messed something up--it's going to do a strlen on ReleaseBuffer, so I have to pass length to ReleaseBuffer.  And the comment about TCHAR has me worried.  So second stab:

     

    CString GetTempDirectory()

    {
    	int pathLen  = GetTempPath(0, NULL);
    	CString retval;
    	char *pStr = retval.GetBufferSetLength ((pathlen+1) * sizeof (TCHAR));
            GetTempPath(pathLen, pStr);
    	retval.ReleaseBuffer (pathlen * sizeof(TCHAR));
    	return retval;
    }

    I haven't worked with TCHAR before, not 100% sure about those.  I think the newer CString class may take care of TCHAR inline, but I'm going off the VC++ 6.0 docs right now.  Also, CString doesn't count the trailing 0 in its length, but you need to have it there in the buffer because GetTempPath -will- set the null.

  • Saint Waldo (unregistered) in reply to Ryan

    Anonymous:
    Offtopic:  Can we, the people of the internet, please have a moratorium on the "trademark and capitalization" joke?

    "Memory leaks are a Big Deal TM,"
    "It is a Good Thing TM,"  "it is a Bad Thing"

    Just stop it please,  this stopped being funny years ago.

    Your Mom stopped being funny years ago. We'll do something about the annoying "TM" when you do something about her.

     

    Captcha: pacman

    Wakkawakkawakkawakkawakkawakkawak-beryooyooyooyoop-boink-boink.

  • Rich (unregistered) in reply to Bob Janova
    Bob Janova:
    The Real WTF (TM)* is that you need to write code like this (with pointers and stuff, not necessarily with WTFs) to get information out of the OS in the first place.

    <font size="1">*: Yes, I did just write this because we were all asked to stop it. Sue me, I'm a contrarian.</font>

     I never like this passing in of buffers anyway. Let me pass in a pointer to a char*, the function can allocate the buffer and set my char* to point to it (or just return it). The WTF is that I have to guess how much space a function is going to use. The bonus is that you can set my pointer to NULL if things screw up which makes for a nice easy test.

     
    In the example of getTempPath, it's pretty obvious that no one in their right mind is going to want a partial path returned, either I get it or I don't. Return a pointer to the path or NULL.

     

    Of course, the other WTF is those functions that return a pointer to some memory but it's the same pointer every time. A second call destroys the original data. To whoever writes this stuff, why? Don't you trust me to free memory? If you don't, do you think it's less likely I'll write some crap code that expects the data to remain untouched? How am I supposed to guarantee that between when I call your function and when I copy the data to another piece of memory, some other thread doesn't call your function and screw me up? What were you thinking?

    Yes, I'm talking to you, localtime()*

    Rant over.

     

    Rich
     

    *Yes, I'm aware of localtime_r(). But still, WTF? 

  • (cs) in reply to Shrek
    Anonymous:

    The real WTF is people still use languages where they have to give a crap about pointers, buffer overruns and orphaned memory. 

    If a large number of programmers do not understand pointers, it is not a sign that pointers need to be removed. It is a sign that there are too many people who are not programmers pretending that they are. It really is that simple. 

  • (cs) in reply to anon the cowardly

    Anon the cowardly:  "Cudos"?  How about "Kudos"?

     

     

  • Anon (unregistered)

    But this is all assuming that GetTempPath and CString are the ones we're all familiar with.

    They may be entirely different classes / functions that take c++ references to pointers :-) 

    In which case the CString could delete path correctly and GetTempPath could do a delete and new allocation on path - now that would be a real WTF.

     

  • (cs) in reply to PseudoNoise
    Anonymous:
     

    Ah crud, just realized I messed something up--it's going to do a strlen on ReleaseBuffer, so I have to pass length to ReleaseBuffer.  And the comment about TCHAR has me worried.  So second stab:

     
    CString GetTempDirectory()

    {

    	int pathLen  = GetTempPath(0, NULL);
    	CString retval;
    	char *pStr = retval.GetBufferSetLength ((pathlen+1) * sizeof (TCHAR));
            GetTempPath(pathLen, pStr);
    	retval.ReleaseBuffer (pathlen * sizeof(TCHAR));
    	return retval;
    }

    That's almost the way I would do it.  For me, I would do it:

    void GetTempDirectory(CString& temp_dir)

    {

    	int pathLen  = GetTempPath(0, NULL);
    	char *pStr = temp_dir.GetBufferSetLength ((pathlen+1) * sizeof (TCHAR));
            GetTempPath(pathLen, pStr);
    	temp_dir.ReleaseBuffer (pathlen * sizeof(TCHAR));
    }
    Of course, that involves changing the interface, but having an object that can be arbitrarily large in size as a return is pretty dangerous.
     
    Ok, WTF is with the no word wrap thing? 
  • Rich (unregistered) in reply to PseudoNoise

    Geez, you guys make it too complicated with all your fancy CPU hogging la-de-da code.

     

    CString GetTempDirectory(){

     

      return CString("c:\windows\temp");


    } 

     

    Done.

     

    Seriously though, here's a real WTF: (It seems)

     

     

    Remarks

    The GetTempPath function checks for the existence of environment variables in the following order and uses the first path found:

    1. The path specified by the TMP environment variable.
    2. The path specified by the TEMP environment variable.
    3. The path specified by the USERPROFILE environment variable.
    4. The Windows directory.

     

     The Windows directory as a temporary path eh? No wonder so much stuff doesn't install without administrator rights.

     

    Rich 

  • (cs) in reply to Bill
    Anonymous:
    Anonymous:
    So a working implementation should be: 
    CString GetTempDirectory()
    {
    	int pathLen  = GetTempPath(0, NULL);
    	char* path = new char[pathLen + 1];
            GetTempPath(pathLen, path);
    	CString cpath(path);
    	delete[] path;
    	return cpath;
    }
     
    Sycophants of the world unite! Because you're such nice people, with lovely personalities, and great taste in clothes.

     So close - give you most credit.  If you change from a char * path to a tchar * (that covers the size of the character depending on your platform) you win... Currently you will Guarantee a buffer overflow.

     Thank you for playing, come again

    Just noticing that my captcha is "Clueless"

    Almost!

    Use wchar * instead of tchar *, and call GetTempPathW (unicode) directly.

    The GetTempPath macro in the windows headers will usually just link to GetTempPathA (ANSI). GetTempPathA just does the conversion to/from unicode (using the current codepage), and use GetTempPathW.

    It's been a while since I used the windows headers, but there's a way to switch all the macros to use unicode API functions directly (maybe #define UNICODE 1). So using tchar * with #define UNICODE 1 might be the same as above.

  • (cs) in reply to PseudoNoise

    PseudoNoise: You still aren't declaring the pStr as a TCHAR*, so it's still not quite right. changing that line to TCHAR *pStr = new TCHAR(pathLen + 1); should take care of it. (Why even bother with that retval CString? That's just another way to sneak in a WTF in the future.)

     

    I would have just thrown this in my earlier post, but it appears we have 30 second edit timers or something equally non-useful. :( 

  • Also Anon (unregistered) in reply to PseudoNoise

    Are you guaranteed that the second call to GetTempPath is going to require the same amount of space as the first call?

  • (cs) in reply to Reweave
    Reweave:

    It's been a while since I used the windows headers, but there's a way to switch all the macros to use unicode API functions directly. Using tchar * with #define UNICODE 1 might also work.

     

    It sure will, because that's the point of TCHAR. If you don't define UNICODE and _UNICODE, then it just becomes a regular char. If you do define those, you get a wchar. That's why there is also a TEXT("blah") macro, so if UNICODE is not defined,  "blah" stays as it is, and if it is defined, "blah" becomes "blah"L.

  • (cs) in reply to Rich

    localtime was added to the std C library back in the day when threading wasn't common, so it didn't matter. You called it, parsed the return value, and went on your merry way. Lots of functions were like that.

     Then when EVIL threading and shared memory became commonplace they had to backpedal and offer re-entrant versions of many functions so that local threads could manage the memory and not rely on the shared data segment of mmap'd libc.so...
     

  • jayh (unregistered) in reply to PseudoNoise

     

     Why not simply do this, and skip all the length fussing and the conditional?

    char* path = new char[MAX_PATH + 1];

     

    Just to save a couple of bytes?

  • Erzengel (unregistered) in reply to Rich

    OI! If you do this, your consumers will be mad.

    After all, my Windows is installed on the D drive. My Temp directory is on my F drive.

    This is why the API exists and if you ignore it you are commiting a WTF.

     

    Captch: Quality, what your code snippet is not. (Is this captcha engine designed with irony-aware?)

  • Matt (unregistered) in reply to HitScan

    In response to the large number of deaths caused by his company's new breakfast cereal, "Fruity Oat-y O's with Arsenic," CEO Bob Worthalot said "If a large number of kids don't understand the difference between the fruity O's and the arsenic O's and know to avoid the latter, it is not a sign that arsenic needs to be removed. It is a sign that there are too many stupid children. It really is that simple."

  • Erzengel (unregistered) in reply to Erzengel

    Oops, I thought I clicked quote. That message was supposed to be this:

    Anonymous:

    Geez, you guys make it too complicated with all your fancy CPU hogging la-de-da code.

     

    CString GetTempDirectory(){

     

      return CString("c:\windows\temp");


    }

     

    Done.

     

    Seriously though, here's a real WTF: (It seems)

     

     

    Remarks
    The GetTempPath function checks for the existence of environment variables in the following order and uses the first path found:

    The path specified by the TMP environment variable.
    The path specified by the TEMP environment variable.
    The path specified by the USERPROFILE environment variable.
    The Windows directory.


     The Windows directory as a temporary path eh? No wonder so much stuff doesn't install without administrator rights.

     

    Rich


    OI! If you do this, your consumers will be mad.

    After all, my Windows is installed on the D drive. My Temp directory is on my F drive.

    This is why the API exists and if you ignore it you are commiting a WTF.

     

    Captch: Quality, what your code snippet is not. (Is this captcha engine designed with irony-aware?)

  • (cs) in reply to jayh
    Anonymous:

     

     Why not simply do this, and skip all the length fussing and the conditional?

    char* path = new char[MAX_PATH + 1];

     

    Just to save a couple of bytes?

    Because MAX_PATH is a compile time setting, which could change for different versions of Windows.   

  • Erzengel (unregistered) in reply to Matt

    Anonymous:
    In response to the large number of deaths caused by his company's new breakfast cereal, "Fruity Oat-y O's with Arsenic," CEO Bob Worthalot said "If a large number of kids don't understand the difference between the fruity O's and the arsenic O's and know to avoid the latter, it is not a sign that arsenic needs to be removed. It is a sign that there are too many stupid children. It really is that simple."

    Playing the "think of the children" card?

    We're talking about a design and industrial setting, not a kids playpen setting. Children are not expected to know the differance between arsenic and fruit (that's their parent's job). Adults are. Programming languages for children (like VB) shouldn't have and don't have pointers. Programming languages for professionals that make high-priority and real-time applications can't let the garbage collector run without risking a robotic arm getting out of sync with the rest of the assembly line and destroying hundreds of products before they can be shut down.

    Granted, low-priority applications like UI can use a GC, but people need to use the right tool for the right job. C++ still has a job to do.

     

    Captcha: clueless. Seriously. Irony-aware software.

Leave a comment on “Investment Advice”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article