Is this program buggy

[Andy]

Hi, all

I wrote a simple program. It runs okay with g++ 3.2.2, g++4.0. However,
it
does not execute correctly with gcc-3.4.2. gcc-3.4.3. Could you help me
to check if
this program is buggy?

// header file

#ifndef _MC_STATE_H_
#define _MC_STATE_H_

#include <sys/types.h>
#include <cstring>
#include <cstdlib>
#include <string>

using namespace std;

class Signature
{
public:
Signature(unsigned char * str, size_t len);
Signature(const Signature& nsig);

~Signature();

Signature operator =(Signature osig);

string to_string();

private:
unsigned char * sig;
size_t len;
};


class State
{
public:
State();
State(const void*, int len);
State(const State&);
~State() throw ();

State& operator = (State&);

bool operator == (State&);
bool operator == (State);

bool operator != (State&);

Signature get_signature();

State operator + (State&);

private:

unsigned char * data;
size_t sz;

bool valid_sig;
unsigned char sig[16];
};

#endif



// the main program

#include <sys/types.h>
#include <cstring>
#include <cstdlib>
#include <openssl/md4.h>
#include "state.hh"
#include <cassert>

using namespace std;

Signature::Signature(unsigned char* str, size_t vlen)
{
sig = new unsigned char[len];
len = vlen;
memcpy(sig, str, len);
}

Signature::Signature(const Signature& nsig)
: len(nsig.len)
{
sig = new unsigned char [len];
memcpy(sig, nsig.sig, len);
}


Signature::~Signature()
{
delete[] sig;
}

Signature Signature:perator = (Signature osig)
{
delete[] sig;
len = osig.len;

sig = new unsigned char [len];
memcpy(sig, osig.sig, len);
return *this;
}

string Signature::to_string()
{
char* buf;
int i;

buf = new char[len * 2+1];
for (i = 0; i < len; i++) sprintf(buf+2*i, "%x", sig);
buf[len*2] = 0;

string retval = string(buf);
return retval;
}

/*****************************************************************************/

State::State()
: data(0), sz(0), valid_sig(false)
{}


State::State(const void * dt, int len)
: valid_sig(false), sz(len)
{
data = new unsigned char [len];
memcpy(data, dt, sz);
}


State::State(const State& nst)
: valid_sig(nst.valid_sig), sz(nst.sz)
{
data = new unsigned char[sz];
memcpy(data, nst.data, sz);
memcpy(sig, nst.sig, sizeof(sig));
}

State::~State() throw()
{
delete [] data;
}


State& State:perator = (State& nst)
{
if (data != 0) delete reinterpret_cast<char*>(data);
sz = nst.sz;
valid_sig = nst.valid_sig;
data = new unsigned char[sz];
memcpy(data, nst.data, sz);
if (valid_sig) memcpy(sig, nst.sig, sizeof(sig));
return *this;
}


bool State:perator == (State & nst)
{
if (sz != nst.sz) return false;

return (memcmp(data, nst.data, sz) == 0);
}


bool State:perator == (State nst)
{
if (sz != nst.sz) return false;
return (memcmp(data, nst.data, sz) == 0);
}


bool State:perator != (State & nst)
{
if (sz != nst.sz) return true;
return (memcmp(data, nst.data, sz) != 0);
}

Signature State::get_signature()
{
if (sz <= 16)
return Signature(data,sz);

MD4(data, sz, sig);
return Signature(sig, sizeof(sig));

}


State State:perator + (State& nst)
{
State retval;
retval.data = new unsigned char [this->sz + nst.sz];
retval.sz = this->sz + nst.sz;
retval.valid_sig = false;

memcpy(retval.data, data, sz);
memcpy(retval.data + sz, nst.data, nst.sz);

assert(retval.sz == this->sz + nst.sz);
return retval;
}



#if 1

#include <iostream>
using namespace std;

int main()
{
char data[] =
"fdfdsafdsafhasdkhfksladhfklsdahfklsdahflksdahfdsahkfkasd";
State st(data, sizeof(data));

cout<<st.get_signature().to_string()<<endl;

char data2[] = "abcd";
State st2(data2, sizeof(data2));
cout<<st2.get_signature().to_string()<<endl;

State st3 = st + st2;

cout<<st3.get_signature().to_string() <<endl;
}

#endif


This program is compile with " g++ file_name.cc -lcrypto"

Thanks!

Andy

 

[Andy]

after more experiments, I found that with sudo command, the program
runs okay with gcc 4.0. But running without root previlege, it will
fail with segment fault.

 

[roberth+news]

| Hi, all

| I wrote a simple program. It runs okay with g++ 3.2.2, g++4.0. However,
| it
| does not execute correctly with gcc-3.4.2. gcc-3.4.3. Could you help me
| to check if
| this program is buggy?

Your program is buggy, and so is your programming style. I recommend
getting a good book on C++ ("Accelerated C++" by Koenig an Moo is often
mentioned in this and other groups.)

Specially:
* Use the STL containers to hold your information, and most bugs will
go away.
* Initalize all members before the constructor's body. (New can be
used here too.) Newer use "new foo[len]" before len is initialized.
* Learn more about the const keyword, when you should make your
arguments and member functions const, and what const T& means.
* Learn also when to use a reference.
* New may fail. This could make your objects in an inconsistent state.
* Couple new with the correct delete. This is easier when using STL.
* In header files, do include only the headers you need in that header,
and never ever say "using namespace x" in it.

 

[roberth+news]

| after more experiments, I found that with sudo command, the program
| runs okay with gcc 4.0. But running without root previlege, it will
| fail with segment fault.

Running a programme that is known to contain bugs, with unlimited access
is just plain stupid.

 

[Andy]

Hi,

I think it is okay to use new in this way here. Since I did not declare
new(nothrow), if new fails, it will throw out an exception.

Andy

 

[roberth+news]

| I think it is okay to use new in this way here. Since I did not declare
| new(nothrow), if new fails, it will throw out an exception.

If new fails in your constructor, no object will be created. But what
if some user of your class chooses to catch the exception from
operator=()?
say:

#include "signature.h"
#include <string>

void foo(Signature& s)
{
try {
s = Signature((unsigned char*)"foobarbaz", sizeof "foobarbaz");
}
catch (...) {
}
//...
std::string st = s.to_string(); //Oops!
}

Making sure that your objects are consistent is a smart thing to do.
But of course, it's just a general tip.

 

[David Hilsee]

Hi, all

I wrote a simple program. It runs okay with g++ 3.2.2, g++4.0. However,
it
does not execute correctly with gcc-3.4.2. gcc-3.4.3. Could you help me
to check if
this program is buggy?

It does have some obvious problems.

// header file

#ifndef _MC_STATE_H_
#define _MC_STATE_H_

#include <sys/types.h>
#include <cstring>
#include <cstdlib>
#include <string>

using namespace std;

class Signature
{
public:
Signature(unsigned char * str, size_t len);
Signature(const Signature& nsig);

~Signature();

Signature operator =(Signature osig);

string to_string();

private:
unsigned char * sig;
size_t len;
};

class State
{
public:
State();
State(const void*, int len);
State(const State&);
~State() throw ();

State& operator = (State&);

bool operator == (State&);
bool operator == (State);

bool operator != (State&);

Signature get_signature();

State operator + (State&);

private:

unsigned char * data;
size_t sz;

bool valid_sig;
unsigned char sig[16];
};

#endif



// the main program

#include <sys/types.h>
#include <cstring>
#include <cstdlib>
#include <openssl/md4.h>
#include "state.hh"
#include <cassert>

using namespace std;

Signature::Signature(unsigned char* str, size_t vlen)
{
sig = new unsigned char[len];
len = vlen;
memcpy(sig, str, len);
}

Signature::Signature(const Signature& nsig)
: len(nsig.len)
{
sig = new unsigned char [len];
memcpy(sig, nsig.sig, len);
}


Signature::~Signature()
{
delete[] sig;
}

Signature Signature:perator = (Signature osig)
{
delete[] sig;
len = osig.len;

sig = new unsigned char [len];
memcpy(sig, osig.sig, len);
return *this;
}

Traditionally, operator= returns a reference and takes a reference-to-const
parameter. Also, if there is a problem allocating memory, your Signature
object could be left in a corrupted state.

Signature& Signature:perator = (const Signature& osig) {
unsigned char * tmpSig = new unsigned char [osig.len];
delete[] sig;

len = osig.len;
sig = tmpSig;

memcpy(sig, osig.sig, len);
return *this;
}

string Signature::to_string()
{
char* buf;
int i;

buf = new char[len * 2+1];
for (i = 0; i < len; i++) sprintf(buf+2*i, "%x", sig);
buf[len*2] = 0;

string retval = string(buf);
return retval;
}

This member function contains a memory leak because it does not bother to
deallocate "buf". It could be more simply written as follows:

string Signature::to_string()
{
char buf[3];
string retval;
for (int i = 0; i < len; i++) {
sprintf(buf, "%x", sig);
retval += buf;
}

return retval;
}

However, there really isn't a guarantee that a single unsigned char will be
represented in hex by less than three characters on all platforms. Also,
your original code ignores the case where the value at sig is represented
in hex as only one character. In that case, your code could place a null
character in the middle of "buf", which would indicate the end of the
string, which is probably not what you want. To ensure that the string
generated by sprintf is always at least two characters long, use "%0.2x"
instead of "%x". That way you get a leading zero when the hex
representation is just one character.

/***************************************************************************
**/

State::State()
: data(0), sz(0), valid_sig(false)
{}


State::State(const void * dt, int len)
: valid_sig(false), sz(len)
{
data = new unsigned char [len];
memcpy(data, dt, sz);
}


State::State(const State& nst)
: valid_sig(nst.valid_sig), sz(nst.sz)
{
data = new unsigned char[sz];
memcpy(data, nst.data, sz);
memcpy(sig, nst.sig, sizeof(sig));
}

State::~State() throw()
{
delete [] data;
}


State& State:perator = (State& nst)
{
if (data != 0) delete reinterpret_cast<char*>(data);
sz = nst.sz;
valid_sig = nst.valid_sig;
data = new unsigned char[sz];
memcpy(data, nst.data, sz);
if (valid_sig) memcpy(sig, nst.sig, sizeof(sig));
return *this;
}

Again, allocate first and then copy/destroy to avoid corrupting your objects
in the event of a memory allocation failure. Also, that reinterpret_cast
should be removed because the "data" member was initialized using new
unsigned char[], and testing for a null "data" pointer is unnecessary
because nothing will be done in that case.

State& State:perator = (const State& nst)
{
unsigned char * tmpData = new unsigned char[nst.sz];
delete[] data;

sz = nst.sz;
valid_sig = nst.valid_sig;
data = tmpData;

memcpy(data, nst.data, sz);
if (valid_sig) memcpy(sig, nst.sig, sizeof(sig));
return *this;
}

bool State:perator == (State & nst)
{
if (sz != nst.sz) return false;

return (memcmp(data, nst.data, sz) == 0);
}


bool State:perator == (State nst)
{
if (sz != nst.sz) return false;
return (memcmp(data, nst.data, sz) == 0);
}


bool State:perator != (State & nst)
{
if (sz != nst.sz) return true;
return (memcmp(data, nst.data, sz) != 0);
}

Signature State::get_signature()
{
if (sz <= 16)
return Signature(data,sz);

MD4(data, sz, sig);
return Signature(sig, sizeof(sig));

}

Here you are using MD4, a function that I'm not familiar with, so I can't
comment on it. Also, would "sig" be valid in this case? You never seem to
assign "valid_sig" a value of true, as far as I can tell.

State State:perator + (State& nst)
{
State retval;
retval.data = new unsigned char [this->sz + nst.sz];
retval.sz = this->sz + nst.sz;
retval.valid_sig = false;

memcpy(retval.data, data, sz);
memcpy(retval.data + sz, nst.data, nst.sz);

assert(retval.sz == this->sz + nst.sz);
return retval;
}



#if 1

#include <iostream>
using namespace std;

int main()
{
char data[] =
"fdfdsafdsafhasdkhfksladhfklsdahfklsdahflksdahfdsahkfkasd";
State st(data, sizeof(data));

cout<<st.get_signature().to_string()<<endl;

char data2[] = "abcd";
State st2(data2, sizeof(data2));
cout<<st2.get_signature().to_string()<<endl;

State st3 = st + st2;

cout<<st3.get_signature().to_string() <<endl;
}

#endif

HTH.

 

[David Hilsee]

This member function contains a memory leak because it does not bother to
deallocate "buf". It could be more simply written as follows:

string Signature::to_string()
{
char buf[3];
string retval;
for (int i = 0; i < len; i++) {
sprintf(buf, "%x", sig);
retval += buf;
}

return retval;
}

However, there really isn't a guarantee that a single unsigned char will be
represented in hex by less than three characters on all platforms. Also,
your original code ignores the case where the value at sig is represented
in hex as only one character. In that case, your code could place a null
character in the middle of "buf", which would indicate the end of the
string, which is probably not what you want. To ensure that the string
generated by sprintf is always at least two characters long, use "%0.2x"
instead of "%x". That way you get a leading zero when the hex
representation is just one character.

While we're at it, here's a better version that uses C++ stringstreams
(<sstream>) and manipulators (<iomanip>) to generate the string. Its
behavior isn't undefined on systems where an unsigned char can exceed 2
characters in hex (although it still may not have the desired behavior).

string Signature::to_string()
{
std:stringstream oss;
oss << std::hex << std::setfill('0');
for ( int i = 0; i < len; ++i ) {
oss << std::setw(2) << (int)sig;
}
return oss.str();
}

 

[Andy]

That does make senste. However, is there any good solution for this
kind of object corruption? As for the code you give out, why not just
put s.to_string in try scope?

void foo(Signature& s)
{
try {
s = Signature((unsigned char*)"foobarbaz", sizeof "foobarbaz");
//...
std::string st = s.to_string(); //Oops?
}
catch (...) {
}

}

 

[roberth+news]

| That does make senste. However, is there any good solution for this
| kind of object corruption? As for the code you give out, why not just
| put s.to_string in try scope?

Doesn't really matter. The s.~Signature() will sooner or later be
called, and then delete will be called for an already deleted pointer.
The other member functions also assume that there is a valid pointer; it
is destined to go wrong. Writing correct code is easier if all member
functions leave the object in a consistent state.