Unhandled exception at 0x0FC9E559 (ucrtbased.dll) in MyString.exe: 0xC0000005: Access violation writing location 0x00000000

-1

I have a problem in my string class: "Unhandled exception in strcpy function". I do not have much experience in pointers. Please give me your recommendation. Thank you in advance !

IDE used: Visual Studio 2017

Unhandled exception at 0x0FC9E559 (ucrtbased.dll) in MyString.exe: 0xC0000005: Access violation writing location 0x00000000.

MyString.cpp:

#pragma warning(disable:4996)
#include "MyString.h"
#include "cstring"
#include <iostream>
using namespace std;

MyString::MyString()
{
    length = 0;
    content = NULL;
}

MyString::MyString(int length, const char* content) 
{
    this->length = length;
    this->content = new char[this->length + 1];
    strcpy(this->content, content);
}

MyString::MyString(const char* content)
{
    length = strlen(content);
    this->content = new char[length + 1];
    strcpy(this->content, content);
}

void MyString::setLength(int length) 
{
    this->length = length;
}

const int MyString::getLength() 
{
    return length;
}

void MyString::setContent(char* content) 
{
    strcpy(this->content, content); // Unhandled exception !!!
}

const char* MyString::getContent() 
{
    return content;
}

ostream& operator << (ostream& out, const MyString& string)
{

        out << "Content:\n" << string.content << "\n";
        out << "Length:\n" << string.length << "\n";

    return out;
}

const MyString operator+(MyString& string1, MyString& string2)
{
    MyString concatString;
    concatString.setLength(string1.length + string2.length);
    strcat(string1.content, string2.content);
    concatString.setContent(string1.content);

    return concatString;
}

MyString::~MyString()
{
    delete[] content;
}

MyString.h:

#include <iostream>
using namespace std;

class MyString
{
private:
    int length;
    char* content;

public:

    friend ostream& operator << (ostream& out, const MyString& anotherString);

    MyString(); // Constructor fara parametrii
    MyString(int, const char*); // Constructor cu 2 parametrii
    MyString(const char*); // Constructor cu 1 parametru

    friend const MyString operator+(MyString&, MyString&);

    // setters and getters
    void setLength(int);
    const int getLength();
    void setContent(char*);
    const char* getContent();

    // destructor
    ~MyString();
};

Main.cpp:

#include <iostream>
#include "MyString.h"
using namespace std;

int main() {

    MyString string1("---");
    MyString string2("..");

    cout << (string1 + string2);


    system("pause");
    return 1;
}
c++
heap-memory
asked on Stack Overflow Mar 2, 2019 by Cosmin

2 Answers

1

In

const MyString operator+(MyString& string1, MyString& string2)
{
    MyString concatString;
    concatString.setLength(string1.length + string2.length);
    strcat(string1.content, string2.content);
    concatString.setContent(string1.content);

    return concatString;
}

concatString is created empty, and setLength only set the length without (re)allocated content, so you strcpy a null pointer in setContent

you also need to copy and concat in concatString, not in string1

So for instance :

void MyString::setLength(int length) 
{
    if (length > this->length) {
      char * b = new char[length + 1];

      if (this->content != NULL) {
        strcpy(b, this->content);
        delete [] this->content;
      }
      this->content = b;
    }
    this->length = length;
}

const MyString operator+(const MyString& string1, const MyString& string2)
{
    MyString concatString;
    concatString.setLength(string1.length + string2.length);
    strcpy(concatString.content, string1.content);
    strcat(concatString.content, string2.content);

    return concatString;
}

setContent cannot just do a strcpy, better to do for instance

void MyString::setContent(char* content) 
{
  if (content == NULL) {
    if (this->content != NULL) 
      delete [] this->content;
    this->content = NULL;
    this->length = 0;
  }
  else {
    setLength(strlen(content));
    strcpy(this->content, content);
  }
}

After these two changes, compilation and execution :

pi@raspberrypi:/tmp $ g++ -pedantic -Wextra -g MyString.cpp Main.cpp 
In file included from MyString.cpp:2:0:
MyString.h:22:25: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
     const int getLength();
                         ^
MyString.cpp:41:31: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
 const int MyString::getLength()
                               ^
In file included from Main.cpp:2:0:
MyString.h:22:25: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
     const int getLength();

pi@raspberrypi:/tmp $ ./a.out
Content:
---..
Length:
5
sh: 1: pause: not found

and under valgrind

pi@raspberrypi:/tmp $ valgrind ./a.out
==6134== Memcheck, a memory error detector
==6134== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==6134== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==6134== Command: ./a.out
==6134== 
Content:
---..
Length:
5
sh: 1: pause: not found
==6134== 
==6134== HEAP SUMMARY:
==6134==     in use at exit: 0 bytes in 0 blocks
==6134==   total heap usage: 5 allocs, 5 frees, 21,261 bytes allocated
==6134== 
==6134== All heap blocks were freed -- no leaks are possible
==6134== 
==6134== For counts of detected and suppressed errors, rerun with: -v
==6134== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 6 from 3)

To not have the warning during the compilation do not return const int but just int

It is better to have length to be a size_t rather than a int

getLength and getContent can be const (int getLength() const and const char* getContent() const)


As Christophe says in a remark operator+ returns a copy of the string and you do not define the copy constructor, nor the operator=. When a class contains pointers it is needed to define them, and with recent C++ also the move

answered on Stack Overflow Mar 2, 2019 by bruno • edited Mar 2, 2019 by bruno
1

There are a couple of problems with this code.

First, you need to implement the rule of 3, so also providing a copy constructor and an assignment operator.

Then setLength() adjust the maximum length of your string, but it fails to allocate anything, so that you may create a buffer overflow, or in case the default constructor was used an UB because of the nullptr. This is what happens in your operator+().

Once you have implemented the rule of 3, a quick fix for operator+ could be:

const MyString operator+(MyString& string1, MyString& string2)
{
    MyString concatString(string1.length + string2.length, string1.content);
    strcat(concatString.content, string2.content);

    return concatString;   // but this requires copy constructor to work
}

You for your length-based constructor, you assule that length is larger than the string that you want to copy. So either you assert this, or you use strncpy()

answered on Stack Overflow Mar 2, 2019 by Christophe • edited Mar 2, 2019 by Christophe

User contributions licensed under CC BY-SA 3.0