Incidencia #44312

Check usability of all objects that can change after signals

Abrir Fecha: 2022-04-10 08:16 Última actualización: 2022-06-11 21:51

Informador:
Propietario:
(Ninguno)
Tipo:
Estado:
Open
Componente:
Hito:
Prioridad:
5 - Medium
Gravedad:
5 - Medium
Resolución:
Ninguno
Fichero:
4

Details

That problem is related to the realization of #44216 but actually parts of it (except player complete removal that is now impossible by scripts) already present in 3.0 branch. We must check after any direct or indirect call of any signal that any pointer on a unit, a city (or a player) we have obtained before the callback is valid when we use it after, and if so, that this object is still in the scope it is supposed to be (player is alive, city belongs to the same owner etc.) Maybe we have to rewrite some iterators in some places.

Note: generic existence test for a city or a unit is comparing pointer at their saved index to their saved pointer. For players, this test is not 100% reliable since player indices may be reused but at least it guarantees that we get some valid player pointer, and the probability of creating new player object at the same memory address is low on most OSs.

Ticket History (3/29 Histories)

2022-04-10 08:16 Updated by: ihnatus
  • New Ticket "Check usability of all objects that can changeafter signals" created
2022-04-10 08:18 Updated by: ihnatus
  • Details Updated
2022-04-11 07:17 Updated by: ihnatus
Comentario

Looked through a bit, there are loads of work... Possibly we should divide the task to keep the patches reviewable, e.g. one patch for checking just existence and next one for more sophisticated checks.

2022-04-27 07:18 Updated by: ihnatus
Comentario

I have currently done about 50% of 3.0 patch (without testing). Some parts I leave to handle in the mentioned HRM ticket since those cycles can't be resolved for good with scripts actively meddling.

2022-05-03 04:15 Updated by: ihnatus
  • File 3.0-safe-pointers-after-callbacks.patch (File ID: 9143) is attached
2022-05-03 04:17 Updated by: ihnatus
Comentario

Uff, made a patch for 3.0, have not even compiled it yet. Next I'll need a terrible script to test removing all possible things in all possible cases, and to make the autogame live long enough for these cases, I'll need probably HRM#849859 compiled before. And then, the other branches...

2022-05-04 07:36 Updated by: ihnatus
Comentario

There is a patch and a function that is intended to test it being tied to all the callbacks. I tied it to "unit_moved" only but that broke AI civil units, so no cities testing (but nothing has failed due to dangling pointers til the explorers crept around).

2022-05-04 09:44 Updated by: cazfi
Comentario

- search_homecity_for_caravan(): It might be better to set 'alive' rather than to return immediately - if the unit IS still alive, there might be more code added after that block in the future
- Should we think something else for those comments about a HRM ticket. There's a fair chance that the comments outlive the hrm ticket availability. Maybe that's one ticket that we should just move to osdn for having osdn ticket number for the comments?
- citytools.h: Is the indentation of the parameter lines of the transfer_city_units() wrong (and having tabs?) like it seems reading the patch? If you touch it anyway (for removing trailing space?), might as well fix that.

Overall this should make a great improvement to the situation. Hopefully there's not too big conflicts in porting it to later branches.

--

For future autogame (regression) testing it would be great if we had a ruleset with a lua script doing all kind of nasty things, or actively using lua script in general.

2022-05-04 10:09 Updated by: cazfi
Comentario

btw. The script does not apply against current S3_0. Seems like minor rebase needed.

2022-05-04 18:25 Updated by: ihnatus
Comentario

Reply To cazfi

- Should we think something else for those comments about a HRM ticket. There's a fair chance that the comments outlive the hrm ticket availability. Maybe that's one ticket that we should just move to osdn for having osdn ticket number for the comments?

Well, your suggestions? I just need some pattern to mark the code that need further dealing with.

2022-05-04 18:26 Updated by: ihnatus
  • File 3.0-safe-pointers-after-callbacks.patch (File ID: 9143) is deleted
2022-05-05 11:58 Updated by: cazfi
Comentario

Reply To ihnatus

Reply To cazfi

- Should we think something else for those comments about a HRM ticket. There's a fair chance that the comments outlive the hrm ticket availability. Maybe that's one ticket that we should just move to osdn for having osdn ticket number for the comments?

Well, your suggestions? I just need some pattern to mark the code that need further dealing with.

All the options have their downsides, and this is not very important anyway -> after thinking a while, my suggestion would be that we live with it as it's in the current patch. (my original comment was a question, not a request for a change)

2022-05-06 06:24 Updated by: ihnatus
Comentario

Reply To cazfi

All the options have their downsides, and this is not very important anyway -> after thinking a while, my suggestion would be that we live with it as it's in the current patch. (my original comment was a question, not a request for a change)

Ok, this is not changed. Other your notices are accepted.

2022-05-06 14:08 Updated by: cazfi
Comentario

You know that your process of first developing patch for the most stable branch and then going towards development master is backwards compared to the usual way? Not that it's any of my business, if that's the way you want to work, and with some fixes it does make sense (esp. if you can reproduce the problem in specific branch only). Only that we now have 3.0 patch waiting already, but we have no version to have in testing in unstable branches.

2022-05-06 18:23 Updated by: ihnatus
Comentario

Reply To cazfi

You know that your process of first developing patch for the most stable branch and then going towards development master is backwards compared to the usual way? Not that it's any of my business, if that's the way you want to work, and with some fixes it does make sense (esp. if you can reproduce the problem in specific branch only). Only that we now have 3.0 patch waiting already, but we have no version to have in testing in unstable branches.

I did it this way because number of signals grows in further versions and I wanted to start with a simpler task. Testing all these places already needs a lot of loop jumping. Probably I'll first make the HRM patch for 3.0 to not bother bypassing still dangerous code parts and test them together, and then will gradually advance both to the newer versions.

2022-05-07 02:45 Updated by: cazfi
Comentario

Reply To ihnatus

I did it this way because number of signals grows in further versions and I wanted to start with a simpler task.

The one option to get something in quicker would be to limit the scope of this ticket to just those signals that are present in 3.0, i.e., to port just current patch to later branches without adding handling for more signals in them. Those new signals could be handled in a separate ticket.

FYI. For exposing the current patch to some test use, I'll apply it to my local S3_0-work branch, and will also apply it to autogame runner for the next S3_0 testing round (my autogame runners are currently busy, mainly with with S3_1-d3f / S3_1-alpha3 stuff, but maybe next week I can have another S3_0 run)

2022-05-15 12:18 Updated by: cazfi
  • Summary Updated
Comentario

Reply To cazfi

FYI. For exposing the current patch to some test use, I'll apply it to my local S3_0-work branch, and will also apply it to autogame runner for the next S3_0 testing round (my autogame runners are currently busy, mainly with with S3_1-d3f / S3_1-alpha3 stuff, but maybe next week I can have another S3_0 run)

I have it in S3_0 autogame test, but currently not in my own S3_0-work tree as it does not apply with some other patches there (so another rebase will be needed in the future)

2022-05-15 13:17 Updated by: cazfi
Comentario

Reply To cazfi

I have it in S3_0 autogame test

All the autogames had quickly failed on srv_main.c:1297 call of sanity_check(): "assertion 'pplayers_allied(((punit)->owner), city_owner(pcity))' failed."

I think that's from sanitycheck.c line 211; unit_owner(punit) macro is defined as "(punit)->owner"

2022-05-16 06:10 Updated by: ihnatus
Comentario

Reply To cazfi

I think that's from sanitycheck.c line 211; unit_owner(punit) macro is defined as "(punit)->owner"

Hm, some function in city conquest code exits before transferring the city, can't find the place but it's something stupid...

2022-05-16 06:22 Updated by: cazfi
Comentario

Reply To ihnatus

Reply To cazfi

I think that's from sanitycheck.c line 211; unit_owner(punit) macro is defined as "(punit)->owner"

Hm, some function in city conquest code exits before transferring the city, can't find the place but it's something stupid

Just reading what the patch changes in unit_conquer_city(), this seems wrong:

+ city_remains = player_city_by_number(pplayer, saved_id)
+ && transfer_city(pplayer, pcity, 0, TRUE, TRUE, TRUE,
+ !is_barbarian(pplayer));

First you check that city is *already* owned by the same player (pplayer) that you then transfer the city to.

2022-05-18 05:29 Updated by: ihnatus
Comentario

Reply To cazfi

Just reading what the patch changes in unit_conquer_city(), this seems wrong: + city_remains = player_city_by_number(pplayer, saved_id)
+ && transfer_city(pplayer, pcity, 0, TRUE, TRUE, TRUE,
+ !is_barbarian(pplayer));
First you check that city is *already* owned by the same player (pplayer) that you then transfer the city to.

As I have predicted, it was stupid.

2022-05-27 16:59 Updated by: cazfi
Comentario

I've got alien ruleset autogame failure with the latest patch, and I think the patch is to blame. It's the "1 citizens not equal to size (2)" from check_city_size() failing. There's create_city() call with size 2 in the backtrace (before this patch create_city() was always creating size 1 cities)

2022-05-28 07:48 Updated by: ihnatus
Comentario

Reply To cazfi

I've got alien ruleset autogame failure with the latest patch, and I think the patch is to blame. It's the "1 citizens not equal to size (2)" from check_city_size() failing. There's create_city() call with size 2 in the backtrace (before this patch create_city() was always creating size 1 cities)

Probably, this long planned optimization of create_city() must have a separate ticket (any way, it needs rebasing).

EDIT: created, tested for 3.0, #44703

(Edited, 2022-05-29 06:32 Updated by: ihnatus)
2022-06-11 21:51 Updated by: cazfi

Attachment File List

Editar

You are not logged in. I you are not logged in, your comment will be treated as an anonymous post. » Entrar