String allocation trouble with 'new'

Question

When in doubt, turn to Stackoverflow...

I'm having a problem with string allocation. My goal is to store n length of a passed quoted string. I check m_p for null because I think in debug mode MS likes to set the address to 0xcccccccc instead of 0x00000000.

I passed 1 into length. But when I allocate it using new, I'm getting about 15 symbol characters in m_p. How can this be if m_size = lenth + 1 is 2? I would expect it to allocate only two cells. How can I limit it to length + 1?

 String::String(const char *str, int length) { m_size = length + 1; // make room for null-terminated string if (m_p != NULL) m_p = NULL; try { m_p = new char[m_size]; } catch (bad_alloc e) { throw e; } strncpy(m_p, str, m_size); }

Why are you catching the bad_alloc exception then just rethrowing it? — James McNellis
– James McNellis, Commented Aug 20, 2010 at 18:37
if (m_p != NULL) { m_p = NULL; } - now that's a charming memory leak right there. — EboMike
– EboMike, Commented Aug 20, 2010 at 18:39
Are you writing your own string class? I guess see Scott Meyers Effective C++. He builds a string class in many of the examples. — JustBoo
– JustBoo, Commented Aug 20, 2010 at 18:41
How do you check the result of new? What is m_p? A field in your class? Of what type? I don't think it makes sense to check for NULL. Just make an assignment. What's the purpose of catching the exception like this? — Maciej Hehl
– Maciej Hehl, Commented Aug 20, 2010 at 18:42

EboMike · Accepted Answer · 2010-08-20 18:51:08Z

Let me just point out the flaws here:

You don't catch allocation problems with exception handling, you check the return value for NULL.
Why use strncpy if you know the size of source and destination? memcpy is the fastest option here.
The initial values of your variables is undefined and will vary based on platform and configuration. Don't worry about what m_p was before, just assign the value.
m_p is a terrible name. It doesn't say what it is! m_pString at least?
Catching an exception and rethrowing it? What for?
You're not terminating the string. That's the main problem. Is your source string terminated? You seem to end up with an unterminated string.
As pointed out, there are ready-to-use STL classes that do everything for you and have been used for decades.

Actually, the correct C++ way is to check for the badalloc exception (even though not in the way it is used in the question).
Why? There's nothing you can do once you run out of memory, you can't even build a string to write to a log file. Just let the whole thing crash and call it a day.
Well, maybe it just fails on a 5MB string? That leaves enough for a healthy error dialog or message. Trying to handle failed resource allocations gracefully is always good practice.
Holy hell, I missed that. What a terrible way to begin the new life of an object! I feel so sorry for it. It will regret that it was not instantiated from a different class for the rest of its life.

Andreas Brinck · Accepted Answer · 2010-08-20 18:44:57Z

Are you sure that the actual length of str matches length?

http://www.cplusplus.com/reference/clibrary/cstring/strncpy/

No null-character is implicitly appended to the end of destination, so destination will only be null-terminated if the length of the C string in source is less than num.

Karel Petranek · Accepted Answer · 2010-08-20 18:41:34Z

Where do you see the 15 characters? The memory manager might decide to allocate more than 2 bytes (because of optimization). Or there might be another memory allocation after the 2 bytes that by some coincidence also belongs to your program. Even though you see 15 characters in Visual C++ debugger doesn't mean you can safely access these in your code as you are only guaranteed that you have 2 bytes allocated.

I also see some other issues in your code:

if (m_p != NULL) m_p = NULL;

This will cause a memory leak if m_p has already been allocated. You should set m_p to NULL in your constructor and then replace the above line with:

if (m_p != NULL) { delete[] m_p; m_p = NULL; }

If you are using that part of code only in the constructor, simple m_p = NULL; will do the trick.

Also the try .. catch block doesn't make much sense as you re-throw the exception right away. Either process it in the catch block right away or just remove try catch completely.

Thanks for the thoughts. The 15 characters are shown in m_p after calling 'new'. It returns these characters too when I finally output the string in the console. I didn't think constructors would have existing data in a string but that is a good precaution. Actually, if I try delete [] m_p in debug mode, it gives an access error to 0xCCCCCCCC.
Sorry for being a bit unclear here - I added that delete[] in case you used that part of code on an existing string in a different method (when reallocating for a bigger string in assignment operator for example). It of course makes no sense to do this in the constructor where m_p should only be set to NULL.

Mark B · Accepted Answer · 2010-08-20 18:48:17Z

1

Can you use std::string and watch all your string-related bugs just disappear?

answered Aug 20, 2010 at 18:48

Mark B

96.5k10 gold badges113 silver badges198 bronze badges

Comments

a1ex07 · Accepted Answer · 2010-08-20 18:46:32Z

If you want to store n characters of a passed quoted string, you need to take into account that strncpy doesn't append null character if the length of the source string is greater than or equal to the length argument. So you have to take care of it:

strncpy(m_p, str, length); // or strncpy(m_p, str, m_size-1); m_p[length] = '\0'; // or m_p[m_size-1] = '\0'

James Curran · Accepted Answer · 2010-08-20 18:46:20Z

How are you deciding that there are "15 symbol characters in m_p"?

'm_p' (I'm assuming) is just a char *. When you allocate memory with new, it gives you a pointer to that. That memory assign to you may only be 2 bytes long, but there still more memory right after it --- which is now, or will soon be, assigned to someone else.

Now, if you look at *m_p in the debugger (or print it out), it is going to be assumed to be a nul-terminate string, so it will keep printing character until it reaches a 0-byte. Whether or not all of those characters are part of the block assigned to you is irrelevent.

David · Accepted Answer · 2010-08-20 18:48:54Z

I think what you are observing is the debugger displaying what is at memory location pointed to by m_p. The debugger displays the char array characters until it sees a null character which you did not append to the C string, hence the about 15 characters appended you mentioned. Read other answers on your questions, they will help alleviating this effect altogether.

TheUndeadFish · Accepted Answer · 2010-08-20 18:55:40Z

The other answer's are correct about strncpy. That is likely the source of the problem you're asking about.

But I'm concerned about this statement:

I check m_p for null because I think in debug mode MS likes to set the address to 0xcccccccc instead of 0x00000000.

It only sets uninitialized pointers to 0xcccccccc. It does this to help catch places where your code assumes an uninitialized pointer is set to 0x00000000. In C++ you must always remember that primitive types (pointers, char, short, int, float, etc) have no guaranteed value if left uninitialized.

So if you leave that pointer uninitialized in release mode, sometimes is may be 0x00000000 and sometimes it may be garbage values that are not zero. And that will very likely cause problems in what you're trying to do.

if (m_p != NULL) m_p = NULL; is not the solution. The solution is to set m_p to NULL in the constructor of your class. That way you guarantee it has the proper value when you expect it to.

Collectives™ on Stack Overflow

String allocation trouble with 'new'

8 Answers 8

9 Comments

Comments

3 Comments

Comments

Comments

Comments

Comments

Comments

Hot Network Questions

Collectives™ on Stack Overflow

8 Answers 8

9 Comments

Comments

3 Comments

Comments

Comments

Comments

Comments

Comments

Related