[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1695701-militarywindow-crash into lp:widelands
GunChleoc has proposed merging
lp:~widelands-dev/widelands/bug-1695701-militarywindow-crash into lp:widelands.
Commit message:
Fix crash when a militarysite window is open while the enemy conquers it
- Turned buildings in building windows from pointer to reference, so we can
check whether it's still there.
- Die if there is no building or building's owner.
- Refactored out die() function to avoid code duplication.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1695701 in widelands: "Crash when a militarysite window is open while
the enemy conquers it"
https://bugs.launchpad.net/widelands/+bug/1695701
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1695701-militarywindow-crash/+merge/325040
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1695701-militarywindow-crash into lp:widelands.
=== modified file 'src/wui/buildingwindow.cc'
--- src/wui/buildingwindow.cc 2017-04-17 14:13:55 +
+++ src/wui/buildingwindow.cc 2017-06-04 08:48:14 +
@@ -52,7 +52,7 @@
: UI::UniqueWindow(&parent, "building_window", ®, Width, 0, b.descr().descname()),
is_dying_(false),
parent_(&parent),
- building_(b),
+ building_(&b),
workarea_overlay_id_(0),
avoid_fastclick_(avoid_fastclick),
expeditionbtn_(nullptr) {
@@ -67,7 +67,7 @@
}
void BuildingWindow::on_building_note(const Widelands::NoteBuilding& note) {
- if (note.serial == building_.serial()) {
+ if (note.serial == building_->serial()) {
switch (note.action) {
// The building's state has changed
case Widelands::NoteBuilding::Action::kChanged:
@@ -77,13 +77,9 @@
break;
// The building is no more
case Widelands::NoteBuilding::Action::kStartWarp:
- igbase()->add_wanted_building_window(building().get_position(), get_pos(), is_minimal());
+ igbase()->add_wanted_building_window(building()->get_position(), get_pos(), is_minimal());
// Fallthrough intended
case Widelands::NoteBuilding::Action::kDeleted:
- // Stop everybody from thinking to avoid segfaults
- is_dying_ = true;
- set_thinks(false);
- vbox_.reset(nullptr);
die();
break;
default:
@@ -117,6 +113,14 @@
show_workarea();
}
+// Stop everybody from thinking to avoid segfaults
+void BuildingWindow::die() {
+ is_dying_ = true;
+ set_thinks(false);
+ vbox_.reset(nullptr);
+ UniqueWindow::die();
+}
+
/*
===
Draw a picture of the building in the background.
@@ -127,7 +131,7 @@
// TODO(sirver): chang this to directly blit the animation. This needs support for or removal of
// RenderTarget.
- const Image* image = building().representative_image();
+ const Image* image = building()->representative_image();
dst.blitrect_scale(
Rectf((get_inner_w() - image->width()) / 2.f, (get_inner_h() - image->height()) / 2.f,
image->width(), image->height()),
@@ -140,11 +144,13 @@
===
*/
void BuildingWindow::think() {
- if (!igbase()->can_see(building().owner().player_number()))
+ if (!building() || !building()->get_owner() || !igbase()->can_see(building()->owner().player_number())) {
die();
+ return;
+ }
if (!caps_setup_ || capscache_player_number_ != igbase()->player_number() ||
- building().get_playercaps() != capscache_) {
+ building()->get_playercaps() != capscache_) {
capsbuttons_->free_children();
create_capsbuttons(capsbuttons_);
if (!avoid_fastclick_) {
@@ -165,10 +171,10 @@
* \note Children of \p box must be allocated on the heap
*/
void BuildingWindow::create_capsbuttons(UI::Box* capsbuttons) {
- capscache_ = building().get_playercaps();
+ capscache_ = building()->get_playercaps();
capscache_player_number_ = igbase()->player_number();
- const Widelands::Player& owner = building().owner();
+ const Widelands::Player& owner = building()->owner();
const Widelands::PlayerNumber owner_number = owner.player_number();
const bool can_see = igbase()->can_see(owner_number);
const bool can_act = igbase()->can_act(owner_number);
@@ -176,7 +182,7 @@
bool requires_destruction_separator = false;
if (can_act) {
// Check if this is a port building and if yes show expedition button
- if (upcast(Widelands::Warehouse const, warehouse, &building_)) {
+ if (upcast(Widelands::Warehouse const, warehouse, building_)) {
if (Widelands::PortDock* pd = warehouse->get_portdock()) {
expeditionbtn_ =
new UI::Button(capsbuttons, "start_or_cancel_expedition", 0, 0, 34, 34,
@@ -196,7 +202,7 @@
}
});
}
- } else if (upcast(const Widelands::ProductionSite, productionsite, &building_)) {
+ } else if (upcast(const Widelands::ProductionSite, productionsite, building_)) {
if (!is_a(Widelands::MilitarySite, productionsite)) {
const bool is_stopped = productionsite->is_stopped();
UI::Button* stopbtn = new UI::Button(
@@ -223,7 +229,7 @@
} // upcast to productionsite
if (capscache_ &
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands
Continuous integration builds have changed state: Travis build 2272. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/239238741. Appveyor build 2107. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_net_boost_asio-2107. -- https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-boost-asio. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands
Review: Approve compile, test, small review
One nit inline - no need to fix this.
We should get this in now, otherwise we will never get enouht testing coverage.
Notablilis: try pulling the cable at several stepts in the game,
maybe there is one error left, I am no 100% sure.
FAPP tthis is OK for me.
Diff comments:
>
> === modified file 'src/network/network_lan_promotion.cc'
> --- src/network/network_lan_promotion.cc 2017-01-25 18:55:59 +
> +++ src/network/network_lan_promotion.cc 2017-06-03 05:35:52 +
> @@ -19,116 +19,349 @@
>
> #include "network/network_lan_promotion.h"
>
> -#include
> -#include
> +#ifndef _WIN32
> +#include
> +#endif
>
> +#include "base/i18n.h"
> #include "base/log.h"
> -#include "base/macros.h"
> +#include "base/warning.h"
> #include "build_info.h"
> #include "network/constants.h"
>
> +namespace {
> +
> + /**
> + * Returns the IP version.
> + * \param addr The address object to get the IP version for.
> + * \return Either 4 or 6, depending on the version of the given address.
> + */
> + int get_ip_version(const boost::asio::ip::address& addr) {
> + assert(!addr.is_unspecified());
> + if (addr.is_v4()) {
> + return 4;
> + } else {
> + assert(addr.is_v6());
> + return 6;
> + }
> + }
> +
> + /**
> + * Returns the IP version.
> + * \param version A whatever object to get the IP version for.
> + * \return Either 4 or 6, depending on the version of the given address.
> + */
> + int get_ip_version(const boost::asio::ip::udp& version) {
> + if (version == boost::asio::ip::udp::v4()) {
> + return 4;
> + } else {
> + assert(version == boost::asio::ip::udp::v6());
> + return 6;
> + }
> + }
> +}
> +
> /*** class LanBase ***/
> -
> -LanBase::LanBase() {
> -
> - sock = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP); // open the socket
> -
> - int32_t opt = 1;
> - // the cast to char* is because microsoft wants it that way
> - setsockopt(sock, SOL_SOCKET, SO_BROADCAST,
> reinterpret_cast(&opt), sizeof(opt));
> +/**
> + * \internal
> + * In an ideal world, we would use the same code with boost asio for all
> three operating systems.
> + * Unfortunately, it isn't that easy and we need some platform specific code.
> + * For IPv4, windows needs a special case: For Linux and Apple we have to
> iterate over all assigned IPv4
> + * addresses (e.g. 192.168.1.68), transform them to broadcast addresses
> (e.g. 192.168.1.255) and send our
> + * packets to those addresses. For windows, we simply can sent to
> 255.255.255.255.
> + * For IPv6, Apple requires special handling. On the other two operating
> systems we can send to the multicast
> + * address ff02::1 (kind of a local broadcast) without specifying over which
> interface we want to send.
> + * On Apple we have to specify the interface, forcing us to send our message
> over all interfaces we can find.
> + */
> +LanBase::LanBase(uint16_t port)
> + : io_service(), socket_v4(io_service), socket_v6(io_service) {
>
> #ifndef _WIN32
> -
> - // get a list of all local broadcast addresses
> - struct if_nameindex* ifnames = if_nameindex();
> - struct ifreq ifr;
> -
> - for (int32_t i = 0; ifnames[i].if_index; ++i) {
> - strncpy(ifr.ifr_name, ifnames[i].if_name, IFNAMSIZ);
> -
> - DIAG_OFF("-Wold-style-cast")
> - if (ioctl(sock, SIOCGIFFLAGS, &ifr) < 0)
> - continue;
> -
> - if (!(ifr.ifr_flags & IFF_BROADCAST))
> - continue;
> -
> - if (ioctl(sock, SIOCGIFBRDADDR, &ifr) < 0)
> - continue;
> - DIAG_ON("-Wold-style-cast")
> -
> - broadcast_addresses.push_back(
> -
> reinterpret_cast(&ifr.ifr_broadaddr)->sin_addr.s_addr);
> - }
> -
> - if_freenameindex(ifnames);
> + // Iterate over all interfaces. If they support IPv4, store the
> broadcast-address
> + // of the interface and try to start the socket. If they support IPv6,
> just start
> + // the socket. There is one fixed broadcast-address for IPv6 (well,
> actually multicast)
> +
> + // Adapted example out of "man getifaddrs"
> + // TODO(Notabilis): I don't like this part. But boost is not able to
> iterate over
> + // the local IPs and interfaces at this time. If they ever add it,
> replace this code
> + struct ifaddrs *ifaddr, *ifa;
> + int s, n;
> + char host[NI_MAXHOST];
> + if (getifaddrs(&ifaddr) == -1) {
> + perror("getifaddrs");
> + exit(EXIT_FAILURE);
> + }
> + for (ifa = ifaddr, n = 0; ifa != nullptr; ifa = ifa->ifa_next, n++) {
> + if (ifa->ifa_addr == nullptr)
> + continue;
> +
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/gcc7 into lp:widelands
Continuous integration builds have changed state: Travis build 2273. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/239238959. Appveyor build 2108. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_gcc7-2108. -- https://code.launchpad.net/~widelands-dev/widelands/gcc7/+merge/323576 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/gcc7 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1695701-militarywindow-crash into lp:widelands
Review: Needs Fixing Just ran into this (again?), time to get it fixed. Codew review: build all around *building_/building, building_->/building_, building_/&building_ LGTM compiled, have an autosave a minute before being atacked an get a crash at: [Host]: Received ping from metaserver. Forcing flag at (123, 19) Segmentation fault: 11 maikafer:bug-1695701-militarywindow-crash klaus$ Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 widelands 0x00010dbed06f BuildingWindow::think() + 255 (buildingwindow.cc:153) 1 widelands 0x00010daafb03 UI::Panel::do_think() + 51 (panel.cc:458) 2 widelands 0x00010daafb23 UI::Panel::do_think() + 83 (panel.cc:458) Mhh, this is quite some complex code there, Ill simply it and try again -- https://code.launchpad.net/~widelands-dev/widelands/bug-1695701-militarywindow-crash/+merge/325040 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1695701-militarywindow-crash. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1695701-militarywindow-crash into lp:widelands
I simpified the code a bit, but this makes the carsh just appear somewhere else. Gun: do you need my savegame? Will try with a debugger, later -- https://code.launchpad.net/~widelands-dev/widelands/bug-1695701-militarywindow-crash/+merge/325040 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1695701-militarywindow-crash. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1695701-militarywindow-crash into lp:widelands
Review: Needs Fixing
Mhh, thats ood, last "normal" code in Debugger is:
void Panel::do_think() {
if (thinks())
think();
I don get it, where/when is BuildingWindow::building_ set to null?
looks like building_ point to some reclaimed space?
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1695701-militarywindow-crash/+merge/325040
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1695701-militarywindow-crash.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands
Thanks for the explanation and fixing Bunnybot! Good to see that it is working again. I did a short test with two network interfaces on Linux. With the current code one packet is sent on both interfaces, so the kernel (or so) is already dealing with duplicating them. With the apple-code, two packets with the same contents are sent on both interfaces. So as long as it doesn't make any problems, I would keep the current code. Removing/Adding the network cable did not change anything but some time later I also got the exception. The problem is/was that the metaserver-connection gets disconnected, so the client does a DNS lookup to get its IP. When that fails (i.e. currently no network) an exception is thrown. In the handler of this exception the metaserver is disconnected which crashes since we aren't connected anyway (and so have no socket). I pushed a simple fix. But... is it my system or is trunk (and this branch) currently broken? When ingame the view is permanently scrolling down, as if the arrow-down key is pressed. -- https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-boost-asio. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands
Pushed a fix for an assert(state_ == OFFLINE) bug which I encountered. The problem was that on crash of a hosted game the logic jumped back to the main menu without closing the connection to the metaserver. So when the player tries to connect again, he already is connected to the metaserver (and not offline). To reproduce the bug: - Login to the metaserver - Cut your wifi cable - Try to host a game. Should fail and jump you back into the main menu - Try to connect to the metaserver again (I did this one without network) - Game crashes due to the assert -- https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-boost-asio. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands
Ahh. As I have an untrustbale network cable I have seen this already. You changes are fine with me. -- https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-boost-asio. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1695701-militarywindow-crash into lp:widelands
Continuous integration builds have changed state: Travis build 2276. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/239306428. Appveyor build 2111. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1695701_militarywindow_crash-2111. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1695701-militarywindow-crash/+merge/325040 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1695701-militarywindow-crash. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
