What is wrong with my delete[]?

Question

I delete arrays always with delete[]. But HP Fortify shows a Memory Leak for that. What is wrong with my code?

unsigned buflen = SapUcConverter::getFormatBufferLength(len);

char* buffer = new char[buflen]; // Here is the memory leak marked by Fortify

if(valueCanBeLogged) {
    LOGMSG(_DBUG, "nameForLog=%s, len=%d, sapuc='%.*s'",
            nameForLog, len, buflen,
            SapUcConverter::format(buffer, sapuc, len));
} else {
    LOGMSG(_DBUG, "nameForLog=%s, len=#####, sapuc=#####");
}

delete[] buffer;

What does `SapUcConverter::format(buffer, sapuc, len)` do on `buffer` ? comment if clause and check for memory leaks. also check with vld . — alirakiyan, Feb 16 '16 at 13:50
Can your code throw before reaching the `delete[]`? In any case I would suggest you use some kind of wrapper object that handles memory deallocation. — Mohamad Elghawi, Feb 16 '16 at 13:53
Why is the most important part of your question hidden behind scroll bars? Come on! — Kerrek SB, Feb 16 '16 at 13:55
Out of interest, is there a reason you can't use standard STL dynamic containers like `vector`? (e.g. a restricted embedded environment) Probably I'm just not a diverse enough programmer, but I have never needed to use raw `new`/`delete`. Although I'm sure they have their uses where every ounce of control matters. — underscore_d, Feb 16 '16 at 13:56
@alirakiyan SapUcConverter::format() is a long function for building a logging string. There is no throw in it. — thersch, Feb 16 '16 at 13:58
@Galik I have only the saved Fortify result of production of last night. So I cannot try ad hoc tests. I would need to install Fortify on my machine. — thersch, Feb 16 '16 at 14:00
@underscore_d `vector` is a good idea. I don't know why I use just POD. — thersch, Feb 16 '16 at 14:02
`std::vector` is a better idea than `char[buflen]` but still a much worse idea than `std::string`. — caps, Feb 16 '16 at 17:28
@caps Is this also the case if I fill the string with characters one by one. `SapUcConverter::format` is a converter function that converts a 2-byte character string to a ASCII string like so: "abc" with 2-byte-char is converted byte by byte to a ASCII string with printable characters and HEX code "a_b_c_ (61.00 62.00 63.00)". — thersch, Feb 17 '16 at 11:43
Yes, that should be fine. Internally, `std::string` is a heap-allocated `char[]`. If you have char-width problems with `std::string` you will have them with `char[]` also. — caps, Feb 17 '16 at 16:09

eerorika · Accepted Answer · 2016-02-16T14:54:01.717

8

If SapUcConverter::format or any function that might be called when LOGMSG is expanded (assuming it's a macro) is not declared noexcept, then as far as the code that calls them knows, they may throw. And if they do, then buffer leaks. Solution: Adhere to RAII principle. Simplest way to RAII is to use std::vector or std::string.

SapUcConverter::format() is a long function for building a logging string. There is no throw in it.

Just because there isn't a throw expression, doesn't mean that it can't throw. Sounds like it might allocate dynamic memory. new expressions can throw. Appending to a std::string may throw. But if you're 100% that no expression in SapUcConverter::format can throw, then you can use the noexcept specifier.

edited Feb 16 '16 at 14:54

answered Feb 16 '16 at 14:03

eerorika

232,697
12
197
326

Excellent concise summary and recommendations. – underscore_d Feb 16 '16 at 14:13
1

Thanks a lot user2079303. Of course I cannot be sure that there isn't any exception. So I have two options: a) Ignore this Fortify error, b) Use RAII principle (vector or std::string). – thersch Feb 16 '16 at 14:26
@thersch ) If you do ignore it, then do realize that a throw will lead to a leak. Of course, if you have no way of properly handling the throw, then you'll have to terminate anyway so the leak won't matter. b) ...or you can use `unique_ptr` too if that's the way you roll :) – eerorika Feb 16 '16 at 14:33
`unique_ptr` is C++11. I guess we support older platforms (Win, AIX, Solaris, Linux, HPUX) with older compilers. – thersch Feb 16 '16 at 14:41
1

@thersch, I am not even sure what's your question here. **OF COURSE**, you need to use `std::string`. There is **zero** benefits of using manual dynamic array. – SergeyA Feb 16 '16 at 14:43
@SergeyA You mean `std::vector`. – thersch Feb 16 '16 at 14:45
1

@thersch, I mean `std::string`. It is likely to be more performant than `std::vector` due to small string optimization in C++11 and possible CoW in C++03. – SergeyA Feb 16 '16 at 14:46
1

Searching... SSO=Small String Optimization (http://stackoverflow.com/questions/10315041/meaning-of-acronym-sso-in-the-context-of-stdstring), CoW=Copy-on-write (https://en.wikipedia.org/wiki/Copy-on-write) - And again something learned. Thanks. – thersch Feb 16 '16 at 14:58
1

@thersch: If `std::unique_ptr<>` is not an option because of older platforms, consider [`boost::scoped_array<>`](http://www.boost.org/doc/libs/release/libs/smart_ptr/scoped_array.htm) which does the same thing, and works with C++98 compilers. – DevSolar Feb 16 '16 at 15:08

What is wrong with my delete[]?

1 Answers1