widelands

Merge lp:~widelands-dev/widelands/bad_cast into lp:widelands

  1. bad_cast
  2. Merge into trunk
Proposed by SirVer
Status: Merged
Merged at revision: 8224
Proposed branch: lp:~widelands-dev/widelands/bad_cast
Merge into: lp:widelands
Diff against target: 346 lines (+84/-50)
10 files modified
src/editor/map_generator.cc (+7/-4)
src/editor/tools/place_immovable_tool.cc (+5/-2)
src/logic/editor_game_base.cc (+29/-15)
src/logic/editor_game_base.h (+12/-5)
src/logic/map_objects/immovable.cc (+10/-12)
src/logic/map_objects/immovable.h (+5/-4)
src/logic/map_objects/tribes/building.cc (+7/-2)
src/logic/map_objects/tribes/worker.cc (+3/-3)
src/map_io/s2map.cc (+2/-1)
src/scripting/lua_map.cc (+4/-2)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bad_cast
Reviewer Review Type Date Requested Status
TiborB Approve
GunChleoc Approve
Review via email: mp+313283@code.launchpad.net

Commit message

Avoid accessing invalid memory by copying needed data to the stack.

To post a comment you must log in.
Revision history for this message
SirVer (sirver) wrote :

also some tiny renames to make the code a bit easier to understand.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 1768. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/184092656.
Appveyor build 1606. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bad_cast-1606.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Code LGTM, not tested. Some tiny nits.

review: Approve
Revision history for this message
TiborB (tiborb95) wrote :

I tested it, I failed to get a crash so perhaps it is OK now :)

review: Approve
Revision history for this message
SirVer (sirver) wrote :

Thanks for the review! Addressed all comments, gonna merge this now as the bug is critical.

@bunnybot merge

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/editor/map_generator.cc'
2--- src/editor/map_generator.cc 2016-08-04 15:49:05 +0000
3+++ src/editor/map_generator.cc 2016-12-15 20:41:08 +0000
4@@ -95,16 +95,19 @@
5
6 // Set bob according to bob area
7
8- if (set_immovable && (num = bobCategory->num_immovables()))
9- egbase_.create_immovable(
10+ if (set_immovable && (num = bobCategory->num_immovables())) {
11+ egbase_.create_immovable_with_name(
12 fc, bobCategory->get_immovable(static_cast<size_t>(rng.rand() / (kMaxElevation / num))),
13- MapObjectDescr::OwnerType::kWorld);
14+ MapObjectDescr::OwnerType::kWorld, nullptr /* owner */, nullptr /* former_building_descr */
15+ );
16+ }
17
18- if (set_moveable && (num = bobCategory->num_critters()))
19+ if (set_moveable && (num = bobCategory->num_critters())) {
20 egbase_.create_critter(
21 fc, egbase_.world().get_bob(
22 bobCategory->get_critter(static_cast<size_t>(rng.rand() / (kMaxElevation / num)))
23 .c_str()));
24+ }
25 }
26
27 void MapGenerator::generate_resources(uint32_t const* const random1,
28
29=== modified file 'src/editor/tools/place_immovable_tool.cc'
30--- src/editor/tools/place_immovable_tool.cc 2016-08-04 15:49:05 +0000
31+++ src/editor/tools/place_immovable_tool.cc 2016-12-15 20:41:08 +0000
32@@ -58,7 +58,8 @@
33 do {
34 if (!mr.location().field->get_immovable() &&
35 (mr.location().field->nodecaps() & Widelands::MOVECAPS_WALK))
36- egbase.create_immovable(mr.location(), *i);
37+ egbase.create_immovable(mr.location(), *i, Widelands::MapObjectDescr::OwnerType::kWorld,
38+ nullptr /* owner */);
39 ++i;
40 } while (mr.advance(*map));
41 }
42@@ -84,7 +85,9 @@
43 immovable->remove(egbase);
44 }
45 if (!i->empty())
46- egbase.create_immovable(mr.location(), *i);
47+ egbase.create_immovable_with_name(
48+ mr.location(), *i, Widelands::MapObjectDescr::OwnerType::kWorld, nullptr /* owner */,
49+ nullptr /* former_building_descr */);
50
51 ++i;
52 } while (mr.advance(*map));
53
54=== modified file 'src/logic/editor_game_base.cc'
55--- src/logic/editor_game_base.cc 2016-11-17 06:29:48 +0000
56+++ src/logic/editor_game_base.cc 2016-12-15 20:41:08 +0000
57@@ -326,37 +326,51 @@
58 Immovable& EditorGameBase::create_immovable(const Coords& c,
59 DescriptionIndex const idx,
60 MapObjectDescr::OwnerType type,
61- const Building* former_building) {
62- const ImmovableDescr& descr =
63- *(type == MapObjectDescr::OwnerType::kTribe ? tribes().get_immovable_descr(idx) :
64- world().get_immovable_descr(idx));
65- assert(&descr);
66- inform_players_about_immovable(Map::get_index(c, map().get_width()), &descr);
67- return descr.create(*this, c, former_building);
68+ Player* owner) {
69+ return do_create_immovable(c, idx, type, owner, nullptr);
70 }
71
72-Immovable& EditorGameBase::create_immovable(const Coords& c,
73+Immovable& EditorGameBase::create_immovable_with_name(const Coords& c,
74 const std::string& name,
75 MapObjectDescr::OwnerType type,
76- const Building* former_building) {
77+ Player* owner,
78+ const BuildingDescr* former_building_descr) {
79 DescriptionIndex idx;
80 if (type == MapObjectDescr::OwnerType::kTribe) {
81 idx = tribes().immovable_index(name.c_str());
82 if (!tribes().immovable_exists(idx)) {
83 throw wexception(
84- "EditorGameBase::create_immovable(%i, %i): %s is not defined for the tribes", c.x, c.y,
85- name.c_str());
86+ "EditorGameBase::create_immovable_with_name(%i, %i): %s is not defined for the tribes",
87+ c.x, c.y, name.c_str());
88 }
89 } else {
90 idx = world().get_immovable_index(name.c_str());
91 if (idx == INVALID_INDEX) {
92 throw wexception(
93- "EditorGameBase::create_immovable(%i, %i): %s is not defined for the world", c.x, c.y,
94- name.c_str());
95+ "EditorGameBase::create_immovable_with_name(%i, %i): %s is not defined for the world",
96+ c.x, c.y, name.c_str());
97 }
98 }
99- return create_immovable(c, idx, type, former_building);
100-}
101+ return do_create_immovable(c, idx, type, owner, former_building_descr);
102+}
103+
104+Immovable& EditorGameBase::do_create_immovable(const Coords& c,
105+ DescriptionIndex const idx,
106+ MapObjectDescr::OwnerType type,
107+ Player* owner,
108+ const BuildingDescr* former_building_descr) {
109+ const ImmovableDescr& descr =
110+ *(type == MapObjectDescr::OwnerType::kTribe ? tribes().get_immovable_descr(idx) :
111+ world().get_immovable_descr(idx));
112+ assert(&descr);
113+ inform_players_about_immovable(Map::get_index(c, map().get_width()), &descr);
114+ Immovable& immovable = descr.create(*this, c, former_building_descr);
115+ if (owner != nullptr) {
116+ immovable.set_owner(owner);
117+ }
118+ return immovable;
119+}
120+
121
122 /**
123 * Instantly create a ship at the given x/y location.
124
125=== modified file 'src/logic/editor_game_base.h'
126--- src/logic/editor_game_base.h 2016-11-17 06:08:27 +0000
127+++ src/logic/editor_game_base.h 2016-12-15 20:41:08 +0000
128@@ -144,12 +144,13 @@
129 Bob& create_critter(const Coords&, const std::string& name, Player* owner = nullptr);
130 Immovable& create_immovable(const Coords&,
131 DescriptionIndex idx,
132- MapObjectDescr::OwnerType = MapObjectDescr::OwnerType::kWorld,
133- const Building* former_building = nullptr);
134- Immovable& create_immovable(const Coords&,
135+ MapObjectDescr::OwnerType,
136+ Player* owner);
137+ Immovable& create_immovable_with_name(const Coords&,
138 const std::string& name,
139- MapObjectDescr::OwnerType = MapObjectDescr::OwnerType::kWorld,
140- const Building* former_building = nullptr);
141+ MapObjectDescr::OwnerType,
142+ Player* owner,
143+ const BuildingDescr* former_building);
144 Bob& create_ship(const Coords&, int ship_type_idx, Player* owner = nullptr);
145 Bob& create_ship(const Coords&, const std::string& name, Player* owner = nullptr);
146
147@@ -250,6 +251,12 @@
148 // sends notifications about this.
149 void change_field_owner(const FCoords& fc, PlayerNumber new_owner);
150
151+ Immovable& do_create_immovable(const Coords& c,
152+ DescriptionIndex const idx,
153+ MapObjectDescr::OwnerType type,
154+ Player* owner,
155+ const BuildingDescr* former_building_descr);
156+
157 uint32_t gametime_;
158 ObjectManager objects_;
159
160
161=== modified file 'src/logic/map_objects/immovable.cc'
162--- src/logic/map_objects/immovable.cc 2016-12-06 07:35:21 +0000
163+++ src/logic/map_objects/immovable.cc 2016-12-15 20:41:08 +0000
164@@ -329,8 +329,8 @@
165 */
166 Immovable& ImmovableDescr::create(EditorGameBase& egbase,
167 const Coords& coords,
168- const Building* former_building) const {
169- Immovable& result = *new Immovable(*this, former_building);
170+ const BuildingDescr* former_building_descr) const {
171+ Immovable& result = *new Immovable(*this, former_building_descr);
172 result.position_ = coords;
173 result.init(egbase);
174 return result;
175@@ -344,9 +344,9 @@
176 ==============================
177 */
178
179-Immovable::Immovable(const ImmovableDescr& imm_descr, const Widelands::Building* former_building)
180+Immovable::Immovable(const ImmovableDescr& imm_descr, const Widelands::BuildingDescr* former_building_descr)
181 : BaseImmovable(imm_descr),
182- former_building_descr_(former_building ? &former_building->descr() : nullptr),
183+ former_building_descr_(former_building_descr),
184 anim_(0),
185 animstart_(0),
186 program_(nullptr),
187@@ -354,9 +354,6 @@
188 anim_construction_total_(0),
189 anim_construction_done_(0),
190 program_step_(0) {
191- if (former_building != nullptr) {
192- set_owner(former_building->get_owner());
193- }
194 }
195
196 Immovable::~Immovable() {
197@@ -869,9 +866,8 @@
198 if (bob) {
199 game.create_ship(c, type_name, player);
200 } else {
201- Immovable& imm = game.create_immovable(c, type_name, owner_type);
202- if (player)
203- imm.set_owner(player);
204+ Immovable& imm = game.create_immovable_with_name(
205+ c, type_name, owner_type, player, nullptr /* former_building_descr */);
206 }
207 } else
208 immovable.program_step(game);
209@@ -924,7 +920,8 @@
210 probability_to_grow(descr.terrain_affinity(), f, map, game.world().terrains())) {
211 MapObjectDescr::OwnerType owner_type = descr.owner_type();
212 immovable.remove(game); // Now immovable is a dangling reference!
213- game.create_immovable(f, type_name, owner_type);
214+ game.create_immovable_with_name(
215+ f, type_name, owner_type, nullptr /* owner */, nullptr /* former_building_descr */);
216 } else {
217 immovable.program_step(game);
218 }
219@@ -1024,7 +1021,8 @@
220 (new_location.field->nodecaps() & MOVECAPS_WALK) &&
221 logic_rand_as_double(&game) < probability_to_grow(descr.terrain_affinity(), new_location,
222 map, game.world().terrains())) {
223- game.create_immovable(mr.location(), type_name, descr.owner_type());
224+ game.create_immovable_with_name(mr.location(), type_name, descr.owner_type(),
225+ nullptr /* owner */, nullptr /* former_building_descr */);
226 }
227 }
228
229
230=== modified file 'src/logic/map_objects/immovable.h'
231--- src/logic/map_objects/immovable.h 2016-11-22 19:11:01 +0000
232+++ src/logic/map_objects/immovable.h 2016-12-15 20:41:08 +0000
233@@ -140,7 +140,7 @@
234 ImmovableProgram const* get_program(const std::string&) const;
235
236 Immovable&
237- create(EditorGameBase&, const Coords&, const Widelands::Building* former_building) const;
238+ create(EditorGameBase&, const Coords&, const Widelands::BuildingDescr* former_building_descr) const;
239
240 MapObjectDescr::OwnerType owner_type() const {
241 return owner_type_;
242@@ -202,9 +202,10 @@
243 MO_DESCR(ImmovableDescr)
244
245 public:
246- /// If this immovable was created by a building, 'former_building' can be set in order to display
247- /// information about it.
248- Immovable(const ImmovableDescr&, const Widelands::Building* former_building = nullptr);
249+ /// If this immovable was created by a building, 'former_building_descr' can be set in order to
250+ /// display information about it.
251+ Immovable(const ImmovableDescr&,
252+ const Widelands::BuildingDescr* former_building_descr = nullptr);
253 ~Immovable();
254
255 Coords get_position() const {
256
257=== modified file 'src/logic/map_objects/tribes/building.cc'
258--- src/logic/map_objects/tribes/building.cc 2016-12-03 13:32:28 +0000
259+++ src/logic/map_objects/tribes/building.cc 2016-12-15 20:41:08 +0000
260@@ -239,8 +239,9 @@
261 }
262
263 Building::~Building() {
264- if (optionswindow_)
265+ if (optionswindow_) {
266 hide_options();
267+ }
268 }
269
270 void Building::load_finish(EditorGameBase& egbase) {
271@@ -442,10 +443,14 @@
272 void Building::destroy(EditorGameBase& egbase) {
273 const bool fire = burn_on_destroy();
274 const Coords pos = position_;
275+ Player* building_owner = get_owner();
276+ const BuildingDescr* building_descr = &descr();
277 PlayerImmovable::destroy(egbase);
278 // We are deleted. Only use stack variables beyond this point
279 if (fire) {
280- egbase.create_immovable(pos, "destroyed_building", MapObjectDescr::OwnerType::kTribe, this);
281+ egbase.create_immovable_with_name(pos, "destroyed_building",
282+ MapObjectDescr::OwnerType::kTribe, building_owner,
283+ building_descr);
284 }
285 }
286
287
288=== modified file 'src/logic/map_objects/tribes/worker.cc'
289--- src/logic/map_objects/tribes/worker.cc 2016-11-03 07:20:57 +0000
290+++ src/logic/map_objects/tribes/worker.cc 2016-12-15 20:41:08 +0000
291@@ -774,8 +774,8 @@
292
293 Immovable& newimm = game.create_immovable(
294 pos, state.ivar2, state.svar1 == "tribe" ? MapObjectDescr::OwnerType::kTribe :
295- MapObjectDescr::OwnerType::kWorld);
296- newimm.set_owner(get_owner());
297+ MapObjectDescr::OwnerType::kWorld,
298+ get_owner());
299
300 if (action.iparam1 == Action::plantUnlessObject)
301 state.objvar1 = &newimm;
302@@ -847,7 +847,7 @@
303 position,
304 t.get_resource_indicator(
305 rdescr, (rdescr && rdescr->detectable()) ? position.field->get_resources_amount() : 0),
306- MapObjectDescr::OwnerType::kTribe);
307+ MapObjectDescr::OwnerType::kTribe, nullptr /* owner */);
308
309 // Geologist also sends a message notifying the player
310 if (rdescr && rdescr->detectable() && position.field->get_resources_amount()) {
311
312=== modified file 'src/map_io/s2map.cc'
313--- src/map_io/s2map.cc 2016-12-03 13:32:28 +0000
314+++ src/map_io/s2map.cc 2016-12-15 20:41:08 +0000
315@@ -724,7 +724,8 @@
316 if (idx == Widelands::INVALID_INDEX) {
317 throw wexception("Missing immovable type %s", new_immovable_name.c_str());
318 }
319- egbase.create_immovable(location, idx, Widelands::MapObjectDescr::OwnerType::kWorld);
320+ egbase.create_immovable(
321+ location, idx, Widelands::MapObjectDescr::OwnerType::kWorld, nullptr /* owner */);
322 };
323
324 uint8_t c;
325
326=== modified file 'src/scripting/lua_map.cc'
327--- src/scripting/lua_map.cc 2016-12-08 17:27:00 +0000
328+++ src/scripting/lua_map.cc 2016-12-15 20:41:08 +0000
329@@ -990,13 +990,15 @@
330 if (imm_idx == Widelands::INVALID_INDEX)
331 report_error(L, "Unknown world immovable <%s>", objname.c_str());
332
333- m = &egbase.create_immovable(c->coords(), imm_idx, MapObjectDescr::OwnerType::kWorld);
334+ m = &egbase.create_immovable(
335+ c->coords(), imm_idx, MapObjectDescr::OwnerType::kWorld, nullptr /* owner */);
336 } else if (from_where == "tribes") {
337 DescriptionIndex const imm_idx = egbase.tribes().immovable_index(objname);
338 if (imm_idx == Widelands::INVALID_INDEX)
339 report_error(L, "Unknown tribes immovable <%s>", objname.c_str());
340
341- m = &egbase.create_immovable(c->coords(), imm_idx, MapObjectDescr::OwnerType::kTribe);
342+ m = &egbase.create_immovable(
343+ c->coords(), imm_idx, MapObjectDescr::OwnerType::kTribe, nullptr /* owner */);
344 } else {
345 report_error(
346 L, "There are no immovables for <%s>. Use \"world\" or \"tribes\"", from_where.c_str());

Subscribers

People subscribed via source and target branches

to status/vote changes: