CODEWITHSUNDEEP
c++stringvalgrindcoutstrcpy
bagage
bagage

Reputation: 1114

Conditional jump valgrind with char* and << operator

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;
}

Upvotes: 3

Views: 1152

Answers (3)

sehe
sehe

Reputation: 393684

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;
}

Upvotes: 10

Kerrek SB
Kerrek SB

Reputation: 477474

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.

Upvotes: 1

Hugo Corr&#225;
Hugo Corr&#225;

Reputation: 14799

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);
}

Upvotes: -1

Related Questions