1

Can this code block lead to a memory leak?

char * foo = new char [20];
read(STDIN_FILENO, foo, 20);
string bar;
bar.reserve(20);
bar = foo;
delete[] foo;

I think it can not, as we use delete[] to free up chunks of memory. However, object bar may make a difference here. Please share your points.

Chiel
  • 6,006
  • 2
  • 32
  • 57
nathan
  • 754
  • 1
  • 10
  • 24
  • 5
    Please don't try to learn C++ by asking questions on stackoverflow. It will only make everyone miserable, including you. Read a [book](https://stackoverflow.com/questions/388242) instead. – nwp Feb 28 '17 at 15:11
  • 7
    If any of `string bar;`, `bar = foo` or `bar.reserve(20)` throws, you have a memleak. – Jarod42 Feb 28 '17 at 15:12
  • 2
    Homework service alert... – Chiel Feb 28 '17 at 15:15
  • doesn't this belong on code review? –  Feb 28 '17 at 15:17
  • 3
    @Chiel So what? Homework questions are not off-topic or problematic per se, it's just stuff like assignment dumps (too broad) and "Debug plz!" (no MCVE) that needs treatment. But for the aforementioned reasons, not because they are homework. – Baum mit Augen Feb 28 '17 at 15:20
  • @TheWhiteAfrican It could be on code review, but the way it is asked makes it an alright question on SO – user Feb 28 '17 at 15:20
  • @nwp Remind me to not ask you to mentor anyone. – nicomp Feb 28 '17 at 15:32
  • 2
    @nicomp Why is that? I happen to do [mentoring of sorts](https://chat.stackoverflow.com/rooms/116940/c-questions-and-answers) and occasionally people actually learn things which makes me happy. SO Q&A is just not the right place for that. – nwp Feb 28 '17 at 15:40
  • 1
    [Live on Coliru](http://coliru.stacked-crooked.com/a/594e6e77b0e2f736) with an exception in the read function. – megabyte1024 Feb 28 '17 at 16:48

4 Answers4

5

If any of string bar;, bar.reserve(20) or bar = foo throws, you have a memleak.

You may use some smart pointers to avoid that:

auto foo = std::make_unique<char[]>(20);
Jarod42
  • 203,559
  • 14
  • 181
  • 302
0

You do not need reserve, and if one of the 20 bytes you read from stdin is \0 you will fill only a few bytes.

Here is the correct code:

char * foo = new char [20];
read(STDIN_FILENO, foo, 20);
string bar;
bar.assign(foo, 20);
delete[] foo;

...you may have problems with an exception on std::string (constructor or assign), but memory exception IMHO should be handled only if you allocate large quantity of memory, because if an allocation of 20 bytes fails most surely you will not be able to properly handle an exception.

If I had to improve your code, both for speed and safety I'll do it this way:

char foo[20];
int len = read(STDIN_FILENO, foo, sizeof(foo));
string bar;
if (len > 0)
  bar.assign(foo, len);
gabry
  • 1,370
  • 11
  • 26
0

@Jarod42 is correct. You should use smart pointers. However, in order to solve this problem without smart pointers (to understand the theory), try this:

char * foo = new char [20];

try{
    read(STDIN_FILENO, foo, 20);
    string bar;
    bar.reserve(20);
    bar = foo;
    deleted=true;
    delete[] foo;
    }
catch (...){
    delete[] foo;
    throw;

}

If there is an exception in one of the first lines, the exception will be caught, and foo will still be deleted. However, unless you have a compelling reason not to, you should use unique_ptr or shared_ptr

パスカル
  • 479
  • 4
  • 13
0

If you know the buffer size at compile time, you should not allocate it with new. Instead, you should allocate it on the stack, or better yet, read directly into your string's buffer after resizing it. That way you ensure maximum efficiency both code wise and otherwise, including separating allocation from actual reading.

rubenvb
  • 74,642
  • 33
  • 187
  • 332