Crash with operator+=

[Tomhet]

Should I use a temporary buffer?

//main

#include<cstdio>

#include<ala_text.h>

int main(){
    tale a="hue";
        a+=a;
        a+=a;//here

    puts(a.gets());

    gets((new char[1]));

    return 0;
};

//header

#ifndef ALA_TEXT_H
#define ALA_TEXT_H

#include<cstddef>
#include<cstring>
#include<cstdlib>

typedef char                s_char;
typedef const char          cs_char;
typedef const size_t        size_tc;



class tale{
    private:
        size_t           _length;
        s_char*          _buffer;

    public:
        //method
        size_tc length(){
            return _length;
        };

        cs_char*gets(){
            return(cs_char*)_buffer;
        };

        bool empty(){
            return _length==0;
        };

        //operator
        tale&operator+=(tale&text){
            size_t len=text.length();
            _buffer=(s_char*)realloc(_buffer,_length+len+1);
                strcpy(&_buffer[_length],text.gets());
                _length+=len;
            return*this;
        };

        tale&operator+=(cs_char*text){
            size_t len=strlen(text);
            _buffer=(s_char*)realloc(_buffer,_length+len+1);
                strcpy(&_buffer[_length],text);
                _length+=len;
            return*this;
        };

        tale&operator+=(cs_char chr){
            ++_length;
            _buffer=(s_char*)realloc(_buffer,_length+1);
                _buffer[_length-1]=chr;
                _buffer[_length]=0;
            return*this;
        };

        tale&operator=(tale&text){
            _length=text.length();
            _buffer=(s_char*)realloc(_buffer,_length+1);
                strcpy(_buffer,text.gets());
            return*this;
        };

        tale&operator=(cs_char*text){
            _length=strlen(text);
            _buffer=(s_char*)realloc(_buffer,_length+1);
                strcpy(_buffer,text);
            return*this;
        };

        tale&operator=(cs_char chr){
            _length=1;
            _buffer=(s_char*)realloc(_buffer,_length+1);
                _buffer[0]=chr;
                _buffer[1]=0;
            return*this;
        };

        //constructor
        tale(cs_char*text){
            _length=strlen(text);
            _buffer=(s_char*)malloc(_length+1);
                strcpy(_buffer,text);
        };

        tale(cs_char chr){
            _length=1;
            _buffer=(s_char*)malloc(_length+1);
                _buffer[0]=chr;
                _buffer[1]=0;
        };

        tale(){
            _length=0;
            _buffer=(s_char*)malloc(0);
        };

        //destructor
        ~tale(){
            free(_buffer);
        };
};

#endif 

[abhishekm71]

I'm not very sure but one possible error can be:

tale&operator+=(tale&text){
            size_t len=text.length();

            _buffer=(s_char*)realloc(_buffer,_length+len+1); 
           // should be _buffer=(s_char*)realloc(_buffer,len);
           
           strcpy(&_buffer[_length],text.gets());
           // strcat may be more suitable for the requirement.

                _length+=len;
            return*this;
        };

[Tomhet]

still crashes

[abhishekm71]

Have you tried using strcat? try outputting _buffer at each stage to check whether it is being modified correctly.

[sw143]

@vlad - take it easy, I really don't see anything wrong with mixing malloc/realloc with objective code (as long as we work with raw buffers).

@Tomhet:

The problem is the last byte in original (it means before append) _buffer[] here.

//
// Because You call this function with the same object text.gets() means really _buffer.
//
strcpy(&_buffer[_length],text.gets()); // it means _buffer + _buffer in our case 

There is one common byte to destination and source in strcpy.
Zero terminator at _buffer[len] is overwritten by the first appended char, so strcpy does not detect zero terminator in source string corretly.


Try change this code to that:

memcpy(&_buffer[_length], _buffer, len);
_buffer[_length + len] = 0;

[TheIdeasMan]

I agree with Vlad, also prefer to use std::string rather than char arrays & their associated C functions.

It looks like the OP has taken some C code and put it in a class.

[Tomhet]

@vlad from moscow

satan told me

@abhishekm71

thanks the breakpoint tip
there was a data losing therefore fucked everything so I copied the another tale's buffer

        tale&operator+=(tale&text){
            size_t len=text.length();

            _buffer=(s_char*)realloc(_buffer,(_length+=len)+1);
            s_char*cp=(s_char*)malloc(len+1);
                strcpy(cp,text.gets());
                strcat(_buffer,cp);
            free(cp);

            return*this;
        };

[Tomhet]

I don't like to use string

[vlad from moscow]

@Tomhet

I don't like to use string

In any case you should use operator new and delete instead of malloc, realloc and free,

As I said your code has undefined behaviour.

Let consider the operator += and its call

        tale&operator+=(cs_char*text){
            size_t len=strlen(text);
            _buffer=(s_char*)realloc(_buffer,_length+len+1);
                strcpy(&_buffer[_length],text);
                _length+=len;
            return*this;
        };

a+=a;

The both operands represent the same object. So they have the same value of data member _buffer. You are calling realloc in the operator. So the value of _buffer for the right operand becomes invalid because the memory it pointed was freed. After that you are trying to access the memory that was freed.

[TheIdeasMan]

This is starting to look like a trolling topic.

the += operator is just a messed up reincarnation of strcat.

I

[MikeyBoy]

take it easy, I really don't see anything wrong with mixing malloc/realloc with objective code.

Vlad's being rude and unpleasant, but he's also right. It's really not a great idea to mix your memory management models, as it can lead to all sorts of unnecessary confusion.

And for dynamically allocating objects, you need to use new (and delete), to ensure your constructors (and destructors) are called properly. So, much better to just use new and delete all the way through when writing C++.

I don't like to use string

Really? You like all the memory management headaches that come with continuing to use C-style char arrays? You like having to remember to worry about whether or not your strings are NULL-terminated? You like not having the easy-to-use richness of the methods on strings, and the support provided by STL?

Well, if that's the case, then I guess you must enjoy getting these kinds of bugs and crashes.

[sw143]

You are calling realloc in the operator. So the value of _buffer for the right operand becomes invalid because the memory it pointed was freed. After that you are trying to access the memory that was freed.

It's not true, because both source and destination in strcpy are after realloc.

@Tomhet: Problem is one common byte between original source and destination strings
(zero terminator in _buffer[len] is overwritten by the first appended char, so strcpy does not detect zero terminator corretly).
You don't need extra malloc/free or new/delete to solve it.
Did You try my code from few posts before?
Feel free to ask if something is still not understood.

[TheIdeasMan]

@MikeyBoy

It seems that the OP is writing their own string class, albeit in a mixed up way .

If std::string were used, it would completely remove the need for the code in header file, and main would be very trivial.

So I am now not sure what the motivation for writing this code is.

[Tomhet]

sw, works until I not set _length

I solved with copying but if there is a better way (faster) I'm open

[Tomhet]

oh fuck

sorry, I'm stupid, sw

I forgot to realloc

thanks

I'll remove this thread in 20 minutes

[sw143]

@Tomhet: You are welcome.

Vlad from Moscov wrote:
(...) it is a stupid code.
1). Please tell me where did you read that memory for objects in C++ should be allocated with using malloc or realloc?!(...)
In any case you should use operator new and delete instead of malloc, realloc and free,

Little offtop about memory allocation - above tip it's not steal rule.

Until we're working with raw buffers we can still use malloc/free/realloc and it's still correct in C++ (even if uncommon) - @vlad: please don't messy up others in unpleasant way (Tomhet's problem wasn't related with using malloc).

More, for tasks like above (creating own string class) with potentially many reallocations and a lot of low-level work on raw buffers, malloc/realloc is one of the best choice. (Do we really need initialization from new, when we're going to fill whole buffer just after allocation?) So, IHMO Tomhet uses right tools here - there are a lot of commercial code working in similar way.

But, as MikeyBoy said:

And for dynamically allocating objects, you need to use new (and delete), to ensure your constructors (and destructors) are called properly.

when we're going to create instances of objects we must use new/delete, because we need constructor/destructor to be called. And in this context using malloc is formally incorrect.

[cire]

I think the biggest problem here is that source and destination are not allowed to overlap in calls to strcpy (or strcat for that matter,) and they do when one tries to append a string to itself.