Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[hides][filter_self][filter_vision] crashes wenoth (GNA #22064) #1389

Closed
wesnoth-bugs opened this issue May 8, 2017 · 16 comments
Labels

Comments

@wesnoth-bugs
Copy link

@wesnoth-bugs wesnoth-bugs commented May 8, 2017

Original submission by gfgtdf on 2014-05-20

with a stack overflow/infinite recursion

unit::internal_matches_filter -> unit::invisible -> unit::get_ability_bool -> ability_affects_self -> unit::matches_filter

(Reproduced on win7)
Release: 1.13-dev
Priority: 5 - Normal
Severity: 3 - Normal

@wesnoth-bugs

This comment has been minimized.

Copy link
Author

@wesnoth-bugs wesnoth-bugs commented May 8, 2017

Modified on 2014-05-20

gfgtdf changed title: [hides][filter_delf][filter_vision] crashes wenoth -> [hides][filter_self][filter_vision] crashes wenoth

@Wedge009 Wedge009 added WML and removed Windows labels May 9, 2017
jostephd added a commit to jostephd/wesnoth that referenced this issue Sep 6, 2018
@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 6, 2019

Closing as fixed by 39a0654

@Wedge009 Wedge009 closed this Sep 6, 2019
@gfgtdf

This comment has been minimized.

Copy link
Contributor

@gfgtdf gfgtdf commented Sep 6, 2019

are you sure this commit is on master? usually when a commit with Fixes #xxx is on master github woudl close tihs automaticially.

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

@GregoryLundberg GregoryLundberg commented Sep 6, 2019

Commit is not in 1.14 or master.

@Pentarctagon

This comment has been minimized.

Copy link
Member

@Pentarctagon Pentarctagon commented Sep 6, 2019

@jostephd seeing as this is your commit.

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 6, 2019

Ah, sorry, missed that it was on 1.14. I was wondering why it wasn't automatically closed.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

@gfgtdf gfgtdf commented Sep 6, 2019

Ah, sorry, missed that it was on 1.14

actually its only on jostephd s branch ant not even in 1.14

@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 7, 2019

I wonder why I left it there.

I just forward-ported the commit to current master, I confirm it fixes the crash, but my git fetch isn't working right now so could someone commit this please?

patch for master
commit 5fc5582de8644fd6d029c16222a9fb2515b19cd8
Author: josteph <josteph@fastmail.com>
Date:   Thu Sep 6 14:26:18 2018 +0000

    SUF: Fix infinite recursion in [hides][filter_self][filter_vision]

    Fixes #1389

diff --git a/src/units/filter.cpp b/src/units/filter.cpp
index 9c138974019..d8aa2dae546 100644
--- a/src/units/filter.cpp
+++ b/src/units/filter.cpp
@@ -705,7 +705,8 @@ void unit_filter_compound::fill(vconfig cfg)

                                        for (const int viewer : viewers) {
                                                bool fogged = args.context().get_disp_context().get_team(viewer).fogged(args.loc);
-                                               bool hiding = args.u.invisible(args.loc) && args.context().get_disp_context().get_team(viewer).is_enemy(args.u.side());
+                                               // Check is_enemy() before invisible() to prevent infinite recursion in [abilities][hides][filter_self][filter_vision]
+                                               bool hiding = args.context().get_disp_context().get_team(viewer).is_enemy(args.u.side()) && args.u.invisible(args.loc);
                                                bool unit_hidden = fogged || hiding;
                                                if (c["visible"].to_bool(true) != unit_hidden) {
                                                        return true;
test case
diff --git a/data/scenario-test.cfg b/data/scenario-test.cfg
index 784f11193a6..98549b17ec8 100644
--- a/data/scenario-test.cfg
+++ b/data/scenario-test.cfg
@@ -224,6 +224,12 @@ Xu, Xu, Qxu, Qxu, Ql, Ql, Ql, Xu, Xu, Xu, Xu, Xu, Xu, Xu, Xu, Gg, Gg, Gg, Gg, Gg
                 [/object]
             [/modifications]
             [abilities]
+                [hides]
+                    [filter_self]
+                        [filter_vision]
+                        [/filter_vision]
+                    [/filter_self]
+                [/hides]
                 {ABILITY_SKIRMISHER}
                 {ABILITY_ILLUMINATES}
             [/abilities]
@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 7, 2019

For the last hour or so I've been trying to apply this patch without success. I've also tried to find if there's any reference I can cherry-pick from. Since it's just a two-line change I can just apply it myself or wait for you to commit it yourself.

@jostephd jostephd closed this in d513c49 Sep 7, 2019
@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 7, 2019

@Wedge009 Thanks a lot! You really didn't have to go searching for a reference; I didn't push one, or I'd have said so. Anyway, I just tried the fetch/push again and it went through. Thanks again for your trouble.

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 7, 2019

It's all a learning experience.

@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 7, 2019

Should've just been a matter of git apply or patch -p1...

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 7, 2019

Kept getting 'corrupt patch', 'patch does not apply', etc errors.

@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 9, 2019

It works fine here:

$ git status -s
$ git apply
diff --git a/data/scenario-test.cfg b/data/scenario-test.cfg
index 784f11193a6..98549b17ec8 100644
--- a/data/scenario-test.cfg
+++ b/data/scenario-test.cfg
@@ -224,6 +224,12 @@ Xu, Xu, Qxu, Qxu, Ql, Ql, Ql, Xu, Xu, Xu, Xu, Xu, Xu, Xu, Xu, Gg, Gg, Gg, Gg, Gg
                 [/object]
             [/modifications]
             [abilities]
+                [hides]
+                    [filter_self]
+                        [filter_vision]
+                        [/filter_vision]
+                    [/filter_self]
+                [/hides]
                 {ABILITY_SKIRMISHER}
                 {ABILITY_ILLUMINATES}
             [/abilities]
$ git status -s
 M data/scenario-test.cfg
$ 
@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 9, 2019

Because it was your change. ;) Also I suspect that some white-space and/or EOL coding doesn't come through right in a GitHub comment vs a file.

@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 9, 2019

I didn't use a file. I copied the patch from my github comment and pasted it into git apply, just like you must have done. Let's continue this on discord.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.