Explore our sites

Stack Exchange
tour help careers 2.0
Take the tour ×
Stack Overflow is a question and answer site for professional and enthusiast programmers. It's 100% free, no registration required.
up vote 1 down vote favorite

So I'm a pretty big new at C++, so I'm sure this is a relatively simple problem, but I have a legacy C++ app I'm trying to trace a heap corruption problem and have traced it to this function:

void LTrimZeros(CString *pstr)
{
    char *psz1;
    char *psz2;

    if ( pstr->GetLength() == 0 )
        return;


    psz1 = new char[pstr->GetLength() + 1];
    psz2 = psz1;

    strcpy_s( psz2, strlen(psz2), (const char *) *pstr );

    while ( *psz2 == '0' )
    {
        psz2++;
    }

    *pstr = psz2;

    delete [] psz1;

    return;
}

When it tries to delete psz1 it throws a heap corruption error. Again I am pretty new to C++, so I didn't want to try to fix this and accidentally introduce a memory leak, so I thought I'd ask the experts. Alternative solutions of the same function are also fine, as this app was written in like c++4 originally, but now is upgraded to c++11 (Also a brief explanation of why this causes heap corruption would help a lot).

share|improve this question
 
Is that production code then? –  trojanfoe Apr 22 at 15:17
3  
Use std::string period! Get rid of that all pointer mumble jumble. –  Alok Save Apr 22 at 15:18
 
In the whilte loop, is it *psz2 == '0' ? or you mean *psz2 == '\0' –  Ali Apr 22 at 15:19
 
psz1 is never used, psz2 assigns itself to psz1. Have you checked that GetLength() returns an size that is valid? What about strlen? There is no way that will work properly. You already know the size so why not use pstr->GetLength() + 1 as your length? –  user195488 Apr 22 at 15:20
 
Yes it is production code, this app predates std::string I believe it would be a lot of work to replace it all, it is removing 0's not looking for a null terminator, and yes GetLength is returning the proper value. –  Kevin DiTraglia Apr 22 at 15:24
add comment

2 Answers

up vote 3 down vote accepted

strlen(psz2) is reading uninitialised memory so may read beyond the end of your array. This means that the length you pass to strcpy_s will be unpredictable and may result in you writing beyond the end of the memory allocated for psz1.

Assuming the end of your function is valid (I'm not sufficiently familiar with CString to say for sure), you could simply change your strcpy_s line to

strcpy_s( psz2, pstr->GetLength() + 1, (const char *) *pstr );

You may run into problems here with win32 string handling functions that switch between 8 and 16-bit characters depending on the UNICODE and _UNICODE defines. I agree with Alok Save and others that switching to using std::string would be clearer and simpler.

share|improve this answer
 
How about *pstr = psz2;? Does CString support that? Nah. –  trojanfoe Apr 22 at 15:21
 
@trojanfoe, Captain Oblivious I hadn't got that far through the function. I'll reword my answer accordingly. –  simonc Apr 22 at 15:22
 
psz2 = psz1; doesn't that mean that psz2 points to psz1 which was just newed up? –  Tony The Lion Apr 22 at 15:25
1  
I'd love to switch it to std::string, but we're talking 20k+ lines of code that is all using CStrings throughout. I'd sooner just rewrite the app in C#. –  Kevin DiTraglia Apr 22 at 15:34
1  
@KevinDiTraglia Hehe, oops so it is :) –  trojanfoe Apr 22 at 16:45
show 6 more comments
up vote 2 down vote

From MSDN:

errno_t strcpy_s(
   char *strDestination,
   size_t numberOfElements,
   const char *strSource 
);

Here, in your code, you are calling strlen on an uninitialized array, you need to fix it (pass the maximum number of elements the destination buffer can store):

strcpy_s( psz2, strlen(psz2), (const char *) *pstr );
share|improve this answer
1  
Didn't you just copy the bug to pass strlen(psz2) as length parameter to strcpy_s()? –  harper Apr 22 at 15:28
1  
Yes, I am showing exact line with the bug here, not showing a bug fix (left as an exercise to the reader - should be easy, really). –  piokuc Apr 22 at 15:31
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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