0

Well, here again, this time the compiler is showing me a memory error (leak):

otest(18015,0xacae32c0) malloc: * error for object 0x194e734: incorrect checksum for freed object - object was probably modified after being freed. * set a breakpoint in malloc_error_break to debug

I'm using clang with ARC activated and STL, this is a C++ file (.cpp).

What I found: If I comment the "delete" line it runs without problems. It makes me wonder who is freeing my allocated memory (cStr).

Btw. This code takes a query string (arg=abc&arg2=asdb) and returns a map whit those values.

static map<string, string> getDecodedQueryString( string qs ) {
            map<string, string> r;

            qs = qs+"&";

            char key[100], value[100],  * cStr, *ptr;
            cStr = new char[ qs.length() ];
            memcpy( cStr, qs.c_str(), url.length()-1);
            cStr[qs.length()]=0;

            ptr =  strtok(cStr, "&");
            while ( ptr != NULL && sscanf( ptr, "%[^=]=%[^&]", &key, &value ) == 2) { 
                r[key]=value;
                ptr = strtok(NULL, "&");
            }
            delete [] cStr; //leaking?

            return r; 
        }

Thanks

Fantastic Mr Fox
  • 32,495
  • 27
  • 95
  • 175
subzero
  • 3,420
  • 5
  • 31
  • 40
  • 1
    What is `url.length()` and is that different from `qs.length()`? If `url` is longer than `qs` (which I suspect it might be), you're overwriting past the end of your allocated buffer. – Greg Hewgill May 31 '12 at 19:50
  • 3
    The error indicates that it is not a leak, but a stale reference to freed memory. – Mark May 31 '12 at 19:52
  • 3
    `new char[ qs.length() ]` doesn't allocate enough memory to hold the terminator. Fix that and start using std:vector and/or std::array and you'll be on your way. – Captain Obvlious May 31 '12 at 19:52
  • @GregHewgill, a global variable... and that was the "memory leak" source :S – subzero May 31 '12 at 19:53
  • @ChetSimpson, you mean `std::string` I think. – chris May 31 '12 at 19:54
  • @ChetSimpson in this case, I can not. strtok uses a char *. Not sure if there is an easy tokenaizer solution on C++ (without using Boost) – subzero May 31 '12 at 19:55
  • 1
    @subzero, http://stackoverflow.com/questions/236129/how-to-split-a-string-in-c – chris May 31 '12 at 19:55
  • 1
    @chris std::string could certainly be used and is probably more logical in this case...I just didn't bother looking closely enough to care ;) – Captain Obvlious May 31 '12 at 19:57
  • @SubZero There's nothing prevetning you from using the char pointer returned from `string::c_str()` or `vector::data()` – Captain Obvlious May 31 '12 at 20:00
  • @ChetSimpson: `c_str()` returns a `const char *` and `strtok()` takes a `char *`, since it modifies the buffer. – Greg Hewgill May 31 '12 at 20:01
  • 1
    `std::string`, `std::stringstream`, and `std::getline` would work much better here. – andre May 31 '12 at 20:01
  • @ChetSimpson, yes, it is, the returned object, the string::c_str() returns a const char *, but the cast (char *) did the job – subzero May 31 '12 at 20:02
  • @ahenderson same result, more lines – subzero May 31 '12 at 20:04
  • 1
    @subzero, I'm going to say it now: Don't cast your way out of this. Copy the array into a new, non-const one. – chris May 31 '12 at 20:11
  • @chris I just tried it, not sure why it worked. I thought strtok modifies the buffer, but not this time – subzero May 31 '12 at 20:20
  • @subzero, doing `(char *)str.c_str()` is a remnant of C (the cast), and isn't safe. The C++ version of that is `const_cast(str.c_str())`, but even that isn't appropriate here (or in most other cases). The only way you can do this safely and consistently is by copying the returned data to one you can freely use. – chris May 31 '12 at 21:19
  • 2
    strtok *does* modify the data. However, because what you do is UB, it may also work, or look like its working. In C++11 you could use string::data, since it, too, is guaranteed to be nul-terminated. (In the meantime, you could just append a NUL to the string since it's only a local variable.) – eq- May 31 '12 at 21:24
  • @subzero: No, not the same result. It's much harder to get UB using them. And customers like to pay for fewer bugs, not for fewer lines. – sbi Jun 01 '12 at 12:44

3 Answers3

4

The problem is likely in these lines:

    cStr = new char[ qs.length() ];
    memcpy( cStr, qs.c_str(), url.length()-1);
    cStr[qs.length()]=0;

Even without the memcpy() (which may have its own problems due to the length of url, as I mentioned above in a comment), the cStr[qs.length()] = 0 writes one byte past the end of the buffer.

If your compiler has strdup() available (it's nonstandard, but most do), you can replace this with:

cStr = strdup(qs.c_str());
// ...
free(cStr);

This saves you from having to mess around with manually allocating bytes, copying them, null terminating in the right place, etc.

Greg Hewgill
  • 951,095
  • 183
  • 1,149
  • 1,285
  • 1
    Since you have C++ listed as one of your tags, you may want to consider looking into smart pointers (`std::unique_ptr`, `std::shared_ptr`, and `std::weak_ptr` here: http://en.cppreference.com/w/cpp/memory). – void-pointer May 31 '12 at 20:47
3
Str = new char[ qs.length() ];
...
cStr[qs.length()]=0;

That write is invalid, it's one past the end of cStr. If the allocator stored a checksum right after the allocation, and checks it at delete time, you've just stomped on it.

Mat
  • 202,337
  • 40
  • 393
  • 406
2

Something along the lines of below will do the same thing.

std::stringstream ss(qs);
std::string temp;
std::string key;
std::string value;
while(std::getline(ss, temp, '&')) {
   std::stringstream equal(temp);
   std::getline(equal, key, '=');
   std::getline(equal, value, '=');
   r[key] = value;
}
andre
  • 7,018
  • 4
  • 43
  • 75