Incorrect memory access when copying or assigning Recolouring object.

[GoogleCodeExporter]

(Please attach the patch in unified diff format.)
What are the major things which your patch changes / adds?
One major error I have had on my system is that because the copy constructor 
and assignment operator for the Recolouring class overwrites data out of 
bounds, it has caused frequent run-time errors having to do with creating new 
guests.

Against what version did you make your patch?
1079

Please provide any additional information below.
In palette.cpp lines 65 and 76, both the copy constructor and assignment 
operator depend on the std::copy function. The second argument uses 
multidimensional array notation for the supposed "end iterator" for the 
std::copy function. Unfortunately the bounds [5][256] will not be the end of 
the array, but will actually access memory 254 elements outwards which for my 
system was memory being used by other functions having to do with creating 
guests. Instead of [5][256], you need to write [0][0] + 5*256 which will ensure 
reaching the actual end to the array.

Original issue reported on code.google.com by ikani...@gmail.com on 15 Mar 2014 at 9:54

Attachments:

• palette.cpp

[GoogleCodeExporter]

Patch File:

Original comment by ikani...@gmail.com on 15 Mar 2014 at 10:10

Attachments:

• palette.patch

[GoogleCodeExporter]

While it doesn't cause problems here, your patch look like a fix. While reading 
it, I remembered another way to specify the end of an array, but without 
hard-coding the size.

Could you try the following patch, and check whether it fixes your problem as 
well?

I cannot trigger usage of the copy and assignment methods, it seems, so it's 
untestable by me.

It should do the same as your patch, but without the 5*256 hard-coded.

Original comment by Alberth2...@gmail.com on 15 Mar 2014 at 7:20

Attachments:

• palette_endof.patch

[GoogleCodeExporter]

Yep, your patch worked with non-hard coded locations. I am not sure if it would 
work with dynamically allocated arrays vs fixed arrays though. As for the 
trigger for the assignment operator, the line was person.cpp:185 
"this->recolour = person_type_data.graphics.MakeRecolouring(&this->rnd);". 
This line would be triggered when a new guest was about to be created. 

The actual problem though as I found through gdb was that whenever this line 
was run on my machine, the parameter to member function person::activate const 
Point16 &start, would actually change to different values after this line. This 
weird behavior of const violation could be described because the assignment 
operator function would alter addresses it was not meant to. 

Perhaps you are not getting the problem on your machine because it is 
allocating the same memory in different addresses, where different data would 
be overwrote. I am using mingw on windows, which may allocate memory addresses 
differently than what your machine does.

Original comment by ikani...@gmail.com on 16 Mar 2014 at 1:38

[GoogleCodeExporter]

In order to get person.cpp:185 to activate, set a break point at that line and 
simply build a path to the edge of the map. The side of the map that has 
openings for building paths to the complete edge without "invisible fencing" is 
the top left. There are only certain sections that do not have the "invisible 
fencing," so you may have to test every voxel for being able to build a path 
there.

Original comment by ikani...@gmail.com on 16 Mar 2014 at 1:44

[GoogleCodeExporter]

Fixed in r1087, thanks!

Original comment by Alberth2...@gmail.com on 16 Mar 2014 at 11:14

• Changed state: Fixed