Conditional jump valgrind with char* and << operator

Question

I'm coding my version of the String class, but Valgrind whines about my implementation of the << operator for my string. The error is at the wrong line, if I print char by char it works great.

Where am I wrong?

Valgrind error:

==2769== Conditional jump or move depends on uninitialised value(s)

==2769== at 0x4C2AC28: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)

==2769== by 0x4ECAD60: std::basic_ostream >& std::operator<< >(std::basic_ostream >&, char const*) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.17)

==2769== by 0x400BD5: operator<<(std::ostream&, String&) (string.cpp:22)

==2769== by 0x400AAC: main (main.cpp:12)

My << operator for string:

ostream & operator << (ostream & o, String & inS) {
    o << inS._pData << " "; // the wrong line
    return o;
}

My String class:

class String {
    public:
         unsigned _size;
         char *   _pData;
         String();
         String(const char* inCString);
};

Constructor (for char*):

String::String(const char* inCString) {
    _size = strlen(inCString);
    _pData = new char[_size + 1];
    strncpy(_pData, inCString, _size);
}

Main.cpp:

int main(int, char**) {
    String s1("hello");
    cout << s1;
    return 0;
}

Can I ask why you're going to the trouble of re-implementing string? — Benj, Oct 05 '12 at 14:50
Did you obey the [Rule of Three](http://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming))? — Mooing Duck, Oct 05 '12 at 20:42
@MooingDuck : Yup, but not the whole code's here; only the relevant ones. — bagage, Oct 05 '12 at 21:13
@Benj : it's a classwork; first month of cpp for us, why wanted you to know ? — bagage, Oct 05 '12 at 21:15
@Cqnqrd Classwork is a totally legit reason, if you were going to do this in production code, I'd call you crazy ;-) — Benj, Oct 05 '12 at 22:33

sehe · Accepted Answer · 2012-10-05T15:02:23.470

I don't recommend using raw strings like this.

However, the culprit is here:

strncpy(_pData, inCString, _size+1);

Or, alternatively, store the NUL-termination character manually:

_pData[_size] = 0;

By missing the NUL-termination character, the output operation will just keep on running past the end of the string. (The behaviour may happen to look okay, since the character may be nul by chance, depending on compiler, options etc.)

Hint:

consider using C++ style instead of C API's
if you must use C-style char*, at least use stdrup and free

if you insist on doing NUL-terminated strings, consider writing that the C++ way too:

#include <iostream>
#include <vector>

class String
{
public:
    std::vector<char> _data;
    String();
    String(const char* inCString);
};

std::ostream & operator << (std::ostream & o, String const& inS)
{
    o.write(inS._data.data(), inS._data.size());
    return o << ' ';
}

String::String(const char* inCString)
{
    for (const char* it=inCString; it && *it; ++it)
        _data.push_back(*it);
}

int main(int, char**)
{
    String s1("hello");
    std::cout << s1;
    return 0;
}

@cqnqrd You should also be careful to initialize `_pData` with an empty (null-terminated) string in your constructor with no parameters — penelope, Oct 05 '12 at 14:57
Indeed I must use char* (student) but thanks for advices, I'll practice them. — bagage, Oct 05 '12 at 15:38

score 1 · Answer 2 · answered Oct 05 '12 at 14:56

1

Because you fail to write the zero byte at the end. You need:

strncpy(_pData, inCString, _size + 1);
//                              ^^^^

You should always read the manual very carefully with the n-versions of C string functions, since they all have subtly different semantics.

answered Oct 05 '12 at 14:56

Kerrek SB

464,522
92
875
1,084

Oh ok, I need to copy it too. Thanks for the advice, I'll do them carefully now. – bagage Oct 05 '12 at 15:32

score -1 · Answer 3 · answered Oct 05 '12 at 14:54

-1

Notice that you are not explicit initializing your data members, you are assigning a value to them:

...in order to initialize, you should change to:

String::String(const char* inCString) :
    _size(strlen(inCString)),
    _pData(new char[_size + 1])
{
    strncpy(_pData, inCString, _size);
}

answered Oct 05 '12 at 14:54

Hugo Corrá

14,546
3
27
39

This has nothing to do with the OP's problem. – Kerrek SB Oct 05 '12 at 14:57
2

@penelope: no, they're not. ints and pointers are _not_ initialized unless constructed explicitly, a la `C::C() : m_size(), m_ptr() {}`. – xtofl Oct 05 '12 at 14:59
@penelope, they're POD types, and so the default constructor is not called. The _size value is undefined and _pData is pointing to garbage. – Hugo Corrá Oct 05 '12 at 15:01

Conditional jump valgrind with char* and << operator

3 Answers3