Creating an object on the heap using malloc. Char * fields are overwritten

Question

I have a DoublyLinkedList object. I create an instance of it and use the push method.

DoublyLinkedList test; test.push("C++_мультипарадигмальный_1_1983_check");

The part after which everything starts to go wrong:

 void DoublyLinkedList::push(char* parameters) { List_element* tmp = (List_element*)malloc(sizeof(List_element)); *tmp = List_element(parameters); //. . . }

Fields "normal" are normal and filled. Numbers, logical. But char * somehow breaks. Referring to the same address (at least as Visual Studio says) the values in the string for some reason change to something like "EEEEEE"

The constructor looks like this:

 List_element::List_element(char* fields) { char* iterator, *fields_copy = fields, *temp; unsigned i; iterator = strchr(fields_copy, '_'); i = iterator - fields_copy; name = (char*)malloc(sizeof(char)*(i+1)); for (int z = 0; z < i; z++) { name[z] = fields[fields_copy - fields + z]; } name[i] = '\0'; fields_copy = iterator + 1; iterator = strchr(fields_copy, '_'); i = iterator - fields_copy; paradigm = (char*)malloc(sizeof(char)*(i+1)); for (int z = 0; z < i; z++) { paradigm[z] = fields[fields_copy - fields + z]; } paradigm[i] = '\0'; fields_copy = iterator + 1; is_compiled = fields_copy[0] - 48; fields_copy = fields_copy + 2; iterator = strchr(fields_copy, '_'); i = iterator - fields_copy; temp = (char*)malloc(sizeof(char)*(i+1)); for (int z = 0; z < i; z++) { temp[z] = fields[fields_copy - fields + z]; } temp[i] = '\0'; year_created = atoi(temp); fields_copy = iterator + 1; iterator = strchr(fields_copy, '\0'); i = iterator - fields_copy; description = (char*)malloc(sizeof(char)*(i+1)); for (int z = 0; z < i; z++) { description[z] = fields[fields_copy - fields + z]; } description[i] = '\0'; next = nullptr; prev = nullptr; }

In general, I simply process the string, write the values from the fields to the name field. The algorithm is working, I do not even know what the error is. Tell me please.

The biggest suspicion of this line:

 *tmp = List_element(parameters);

The following unit test passes normally:

 TEST_METHOD(Object_List_element_should_be_created_correctly) { //Arrange List_element list_elem("C++_мультипарадигмальный_1_1983_check"); char* name_test = "C++"; char* paradigm_test = "мультипарадигмальный"; bool is_compiled_test = true; short year_test = 1983; char* description_test = "check"; //Act char* name = list_elem.getName(); char* paradigm = list_elem.getParadigm(); bool is_compiled = list_elem.getIs_compiled(); short year_created = list_elem.getYear_created(); char* description = list_elem.getDescription(); //Assert Assert::AreEqual(strcmp(name_test, name), 0); Assert::AreEqual(strcmp(paradigm_test, paradigm), 0); Assert::AreEqual(is_compiled_test, is_compiled); Assert::AreEqual(year_test, year_created); Assert::AreEqual(strcmp(description_test, description), 0); }

UPD:

 class List_element { char* name; char* paradigm; bool is_compiled; short year_created; char* description; List_element* next; List_element* prev; public: List_element(char* fields); ~List_element(); char* getName(); char* getParadigm(); bool getIs_compiled(); short getYear_created(); char* getDescription(); List_element* getNext(); List_element* getPrev(); void setNext(List_element* next); void setPrev(List_element* prev); };

malloc , of course, is not good, but the matter is hardly in it.
But the implementation of List_element worth a look - the assignment operator.
but C ++ is not C. This is justified, at a minimum, by the fact that malloc / free do not call constructors or destructors.
In addition, you should not use char arrays if there is a std :: string, besides VERY good reasons (if you need to create a vulnerability)
In *tmp , you have a NULL pointer indicating at least the already freed memory.

Accepted Answer · 2016-05-21T19:42:17

This is not the answer yet - but it doesn’t fit into a comment ... is not it easier like this?

 List_element::List_element(char* fields) { char* it = strchr(fields, '_'); int n = iterator - fields; name = new char[n+1]; strncpy(name,fields,n); name[n] = '\0'; //. . . }

But I really want to look at the declaration and the assignment operator List_element . I think the problem is there ...

Update No assignment operator. Generated simply reassigned pointers. And if the name is deleted in the destructor, after assigning the name in the copy it indicates where? in, sorry, the hell knows where - an empty place in the memory ...

Update 2

See code -

 class Test { public: Test() { cout << __func__ << endl; } Test(const Test&) { cout << __func__ << endl; } ~Test() { cout << __func__ << endl; } }; int main(int argc, const char * argv[]) { Test * t = (Test*)malloc(sizeof(Test)); *t = Test(); cout << "WTF???\n"; }

This is your *tmp assignment model. As you can see, the List_element you List_element safely List_element after the assignment. And what do you have in *tmp as a result? ...

@Awethon buy / lend / download books C ++ Primer, Professional C ++, The C ++ Programming Language, or, if not at all, for overclocking in isocpp.org/wiki/faq/c
@Awethon Write an assignment statement with copying strings, for example.
@Awethon Yes, just remember to free tmp with free using the pre-explicitly calling destructor.
What can you do - this is the price of giving up new and delete : taking care of everything yourself ...

Answer 2 · 2016-05-21T20:23:27

Objects of classes with a nontrivial construction cannot be correctly created using bare malloc . With proper understanding of the issue, this can be done correctly, but it requires additional targeted efforts (namely, the construction of the object after memory allocation), which are not visible in your code.

Therefore, what is it all about is not clear to me. Your code in its current state is dead, inoperable in principle. "Correct" there is nothing. And to start talking about something, you must first answer the question what malloc does in your code.

List_element * tmp = (List_element *) malloc (sizeof (List_element));
@Awethon: "Work" this can only be due to a heap of "complementary" bugs.
It just seems like it “works” - you just didn’t test it well.
What you mentioned above is an attempt to apply an assignment operator to an unstructured *tmp object.
The assignment operator cannot be applied to an object that is not compacted, especially if the constructor is non-trivial (as in your case).
The assignment operator always waits for the constructed object and, of course, always deals with the release of "old" resources before assigning new ones.
Therefore, it “works” only because it does not know which way to fall.

Creating an object on the heap using malloc. Char * fields are overwritten

2 answers 2

More articles: