Merge lp:~widelands-dev/widelands/bug-1815613-map-origin-segfault into lp:widelands

Proposed by GunChleoc on 2019-02-12
Status: Merged
Merged at revision: 8985
Proposed branch: lp:~widelands-dev/widelands/bug-1815613-map-origin-segfault
Merge into: lp:widelands
Diff against target: 82 lines (+18/-12)
1 file modified
src/logic/map.cc (+18/-12)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1815613-map-origin-segfault
Reviewer Review Type Date Requested Status
hessenfarmer test Approve on 2019-02-18
kaputtnik testing 2019-02-12 Approve on 2019-02-13
Review via email: mp+363080@code.launchpad.net

Commit message

Fix number overflow in Map::set_origin and clean up the function.

Description of the change

The fix is replacing decltype with size_t, because the positive range of int16_t is too small. The rest is cleanup.

To post a comment you must log in.
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4463. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/492316137.
Appveyor build 4251. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1815613_map_origin_segfault-4251.

kaputtnik (franku) wrote :

Good to have a general log output.

Tested and working :-)

review: Approve (testing)
hessenfarmer (stephan-lutz) wrote :

tested and works like a charm

@bunnybot merge

review: Approve (test)
bunnybot (widelandsofficial) wrote :

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 4463. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/492316137.

Toni Förster (stonerl) wrote :

The error was just a timeout in Travis.

@bunnybot merge force

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/logic/map.cc'
2--- src/logic/map.cc 2018-12-13 07:24:01 +0000
3+++ src/logic/map.cc 2019-02-12 17:49:27 +0000
4@@ -330,60 +330,66 @@
5 assert(0 <= new_origin.y);
6 assert(new_origin.y < height_);
7
8+ const size_t field_size = width_ * height_;
9+
10 for (uint8_t i = get_nrplayers(); i;) {
11 starting_pos_[--i].reorigin(new_origin, extent());
12 }
13
14- std::unique_ptr<Field[]> new_field_order(new Field[width_ * height_]);
15- memset(new_field_order.get(), 0, sizeof(Field) * width_ * height_);
16+ std::unique_ptr<Field[]> new_field_order(new Field[field_size]);
17+ memset(new_field_order.get(), 0, sizeof(Field) * field_size);
18
19 // Rearrange The fields
20- // NOTE because of the triangle design, we have to take special care about cases
21+ // NOTE because of the triangle design, we have to take special care of cases
22 // NOTE where y is changed by an odd number
23 bool yisodd = (new_origin.y % 2) != 0;
24 for (Coords c(Coords(0, 0)); c.y < height_; ++c.y) {
25 bool cyisodd = (c.y % 2) != 0;
26 for (c.x = 0; c.x < width_; ++c.x) {
27 Coords temp;
28- if (yisodd && cyisodd)
29+ if (yisodd && cyisodd) {
30 temp = Coords(c.x + new_origin.x + 1, c.y + new_origin.y);
31- else
32+ } else {
33 temp = Coords(c.x + new_origin.x, c.y + new_origin.y);
34+ }
35 normalize_coords(temp);
36 new_field_order[get_index(c, width_)] = operator[](temp);
37 }
38 }
39 // Now that we restructured the fields, we just overwrite the old order
40- for (decltype(width_) ind = 0; ind < width_ * height_; ind++) {
41+ for (size_t ind = 0; ind < field_size; ind++) {
42 fields_[ind] = new_field_order[ind];
43 }
44
45 // Inform immovables and bobs about their new coordinates.
46- for (FCoords c(Coords(0, 0), fields_.get()); c.y < height_; ++c.y)
47+ for (FCoords c(Coords(0, 0), fields_.get()); c.y < height_; ++c.y) {
48 for (c.x = 0; c.x < width_; ++c.x, ++c.field) {
49 assert(c.field == &operator[](c));
50- if (upcast(Immovable, immovable, c.field->get_immovable()))
51+ if (upcast(Immovable, immovable, c.field->get_immovable())) {
52 immovable->position_ = c;
53+ }
54 for (Bob* bob = c.field->get_first_bob(); bob; bob = bob->get_next_bob()) {
55 bob->position_.x = c.x;
56 bob->position_.y = c.y;
57 bob->position_.field = c.field;
58 }
59 }
60+ }
61
62- // Take care about port spaces
63+ // Take care of port spaces
64 PortSpacesSet new_port_spaces;
65 for (PortSpacesSet::iterator it = port_spaces_.begin(); it != port_spaces_.end(); ++it) {
66 Coords temp;
67- if (yisodd && ((it->y % 2) == 0))
68+ if (yisodd && ((it->y % 2) == 0)) {
69 temp = Coords(it->x - new_origin.x - 1, it->y - new_origin.y);
70- else
71+ } else {
72 temp = Coords(it->x - new_origin.x, it->y - new_origin.y);
73+ }
74 normalize_coords(temp);
75- log("(%i,%i) -> (%i,%i)\n", it->x, it->y, temp.x, temp.y);
76 new_port_spaces.insert(temp);
77 }
78 port_spaces_ = new_port_spaces;
79+ log("Map origin was shifted by (%d, %d)\n", new_origin.x, new_origin.y);
80 }
81
82 /*