Memory leak while trying to replace character in string

Question

I'm generating a big char for future passing to a thread with strcpyand strcat. It was all going ok until I needed to substitute all the occurrences of the space for a comma in one of the strings. I searched for the solution to this here

Problem is, now I have a memory leak and the program exits with this message:

_Dumping objects ->
{473} normal block at 0x0091E0C0, 32 bytes long.
 Data: <AMLUH UL619 BKD > 41 4D 4C 55 48 20 55 4C 36 31 39 20 42 4B 44 20 
{472} normal block at 0x049CCD20, 8 bytes long.
 Data: <        > BC ED 18 00 F0 EC 18 00 
{416} normal block at 0x082B5158, 1000 bytes long.
 Data: <Number of Aircra> 4E 75 6D 62 65 72 20 6F 66 20 41 69 72 63 72 61 
{415} normal block at 0x04A0E200, 20 bytes long.
 Data: <                > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
{185} normal block at 0x049DA998, 64 bytes long.
 Data: < O X8   8       > DC 4F BB 58 38 C5 9A 06 38 D3 88 00 00 00 00 00 
PythonPlugin.cpp(76) : {172} normal block at 0x0088D338, 72 bytes long.
Data: < a X  F <)      > DC 61 BB 58 18 BB 46 06 3C 29 8A 06 CD CD CD CD 
Object dump complete._

Here's the code so you can tell me what I'm doing wrong:

Code of the problem:

char* loop_planes(ac){

char *char1=new char[1000];
    for(...){
         strcpy(char1,"Number of Aircrafts\nHour of simulation\n\n");
         string tmp2=fp.GetRoute();
         tmp2.replace(tmp2.begin(),tmp2.end()," ",","); #PROBLEM IS IN THIS LINE

         const char *tmp3=tmp2.c_str();
         strcat(char1,tmp3);

    }
return char1;
}

The fp.GetRoute()is a string like this: AMLUH UL619 BKD UM748 RUTOL

Also, now that I'm talking about memory allocation, I don't want any future problems with memory leaks, so when should I delete char1, knowing that the thread is going to call this function?

I see no problem in the line you have written `#PROBLEM IS IN THIS LINE`. — PaulMcKenzie, Mar 31 '14 at 19:51
Having never used "replace" - I don't think your usage matches any of the prototypes here: http://www.cplusplus.com/reference/string/string/replace/ — PaulMcKenzie, Mar 31 '14 at 19:54
You might also want to replace fragile constructs such as `new char[1000]` with more durable ones such as `std::string st1("...");` and get rid of both `strcpy` and `strcat` in favor of `std::string` constructors and `std::string` `append()`. — Edward, Mar 31 '14 at 20:00
@PaulMcKenzie: It matches the one labeled "range (6)" in that list. Of course, it doesn't do at all what he thinks it does. — Benjamin Lindley, Mar 31 '14 at 20:06

juanchopanza · Accepted Answer · 2014-03-31T20:06:05.457

4

When you call std::string::replace, the best match is a fumction template whose third and fourth parameters are input iterators. So the string literals you are passing are interpreted as the start and end of a range, when they are not. This leads to undefined behaviour.

You can fix this easily by using the algorithm std::replace instead:

std::replace(tmp2.begin(),tmp2.end(),' ',',');

Note that here the third and fourth parameters are single chars.

edited Mar 31 '14 at 20:06

answered Mar 31 '14 at 19:51

juanchopanza

223,364
34
402
480

There is an overload that matches his arguments, but it does not do what he thinks it does, and the way he's using it is undefined behavior. See the 5th overload on this page (not the one numbered 5): http://en.cppreference.com/w/cpp/string/basic_string/replace – Benjamin Lindley Mar 31 '14 at 19:54
@BenjaminLindley Well, yes, something matches after some conversions. There is not exact match is what I meant. I tried to clarify that. – juanchopanza Mar 31 '14 at 19:58
There are no conversions from `const char*` to something else though. It is a template function. Templated on the input iterator type, which in this case is deduced as `const char*`. – Benjamin Lindley Mar 31 '14 at 19:59
@BenjaminLindley Good point, I had completely missed the function template overload. I should have read your first comment more carefully. – juanchopanza Mar 31 '14 at 20:03
Yes that's it. I added `#include ` and used `std::replace`correctly. Thank you very much – João Pereira Mar 31 '14 at 20:56

score 3 · Answer 2 · answered Mar 31 '14 at 20:22

3

The answer from @juanchopanza correctly identifies and fixes the original question, but since you've asked about memory leaks in general, I'd like to additionally suggest that you replace your function with something that doesn't use new or delete or strcpy or strcat.

std::string loop_planes() {
    std::string res("Number of Aircrafts\nHour of simulation\n\n");
    for (...) {
        std::string route = fp.GetRoute();
        std::replace(route.begin(), route.end(), ' ',',');
        res += route;
    }
    return res;
}

This doesn't require any explicit memory allocation or deletion and does not leak memory. I also took the liberty of changing the return type from char * to std::string to eliminate messy conversions.

answered Mar 31 '14 at 20:22

Edward

6,964
2
29
55

Very nice answer mate! But clarify one thing for me: `strcpy` and `strcat`also allocate memory? – João Pereira Mar 31 '14 at 21:00
and also, the `std::string res` won't have any problem when adding a large large amount of characters to it? – João Pereira Mar 31 '14 at 21:07
1

No, `strcpy` and `strcat` don't allocate memory, but they require that the rest of the program has, somewhere, allocated adequate memory. If 1000 characters are allocated and 1007 characters are `strcat`ed into it, you have a classic buffer overrun. You could allocate 2000 bytes, but how do you know that will always be enough? Those kinds of problems are easily defeated by using much more durable C++ classes -- in this case `std::string`. – Edward Mar 31 '14 at 21:09
@JoãoPereira: no problem with large number of characters. Just now I used used the code posted above with a loop that iterated 1000000 times to create a return string that was 27000040 bytes long. It took a while (0.4 seconds on my Linux-based laptop), but worked flawlessly and leaked no memory. – Edward Mar 31 '14 at 21:16
One more question Edward. Is it faster to do this: `res+=string("\n");` or this: `res.append("\n")`? – João Pereira Apr 01 '14 at 17:35
1

@JoãoPereira: I generally don't worry about it unless speed is critical and I've measured and found that string concatenation is the bottleneck. (So far, that's never happened!) Here's the definitive word on your question, however: http://stackoverflow.com/questions/611263/efficient-string-concatenation-in-c – Edward Apr 01 '14 at 17:44
I've just discovered that i need to pass a `char*` to the thread because the function i'm using to pass the value to python only accepts `char*` and not std::string. So now i have two solutions, making the function like i had it `char* loop_planes*` or use yours `std::string`and convert after to `char*`(don't know how). Which do you think is the best way to do this? – João Pereira Apr 01 '14 at 18:34
@JoãoPereira: once you have a `std::string s` you can obtain a `const char *` by invoking `s.data()` but you will probably also need to obtain the length as `s.length()`. See http://www.cplusplus.com/reference/string/string/data/ for details and caveats. – Edward Apr 01 '14 at 18:43

Memory leak while trying to replace character in string

2 Answers2