Ticket #44778

Resolve script dangers around unit_change_owner()

Open Date: 2022-06-08 15:48 Last Update: 2022-06-16 13:29

Reporter:
Owner:
Type:
Status:
Closed
Component:
MileStone:
Priority:
5 - Medium
Severity:
5 - Medium
Resolution:
Fixed
File:
6

Details

A split of #44738.

It should be handled that this function sometimes does not give out a unit due to the callback in wipe_init(). In do_capture_units() stack iteration must be safe. Also, in diplomat_bribe() we should avoid getting into negative gold (what's the alternative?..)

Ticket History (3/15 Histories)

2022-06-08 15:48 Updated by: ihnatus
  • New Ticket "Resolve script dangers around unit_change_owner()" created
2022-06-10 06:39 Updated by: ihnatus
2022-06-10 06:39 Updated by: ihnatus
  • File 3.0-fix-unit_change_owner.patch (File ID: 9440) is deleted
2022-06-10 06:46 Updated by: ihnatus
Comment

A 3.0 patch is ready, it works for simple cases, though really nasty callbacks I could write still fail due to code out of scope (specifically, clearing of vision in server_remove_unit() before calling a callback that makes it not recursive); some changes done in previous patches in HRM849859 and #44312 are needed for it but the patches theselves should be modified to apply over or beneath this one.

2022-06-13 06:29 Updated by: ihnatus
Comment

Prepared a 3.1 patch. It probably should apply to 3.2 but maybe we should rewrite 3.2 unit_change_owner() completely by HRM874201.

2022-06-13 14:55 Updated by: cazfi
Comment

Reply To ihnatus

Prepared a 3.1 patch. It probably should apply to 3.2

Yes, it applies to master too.

---

- non_allied_not_listed_at() header begins with:
/*******************************************************************//*****
There should be no "*//*" part in S3_0, and in later branches it should be almost in the end of the line - unify with the other headers in the same source file (they are not unified over different source files)

- First line of the commit message should be one line summary (that shows up as heading in various places) followed by one empty line, and the full description after that. I've been adjusting the commit messages anyway, but less I need to do that, the better.

2022-06-14 03:22 Updated by: ihnatus
Comment

Fixed. Also, added a notice on stdout (not shown to the players) if the gold is underflown.

2022-06-14 15:26 Updated by: cazfi
  • Owner Update from (None) to cazfi
  • Resolution Update from None to Accepted
2022-06-16 13:26 Updated by: cazfi
Comment

Reply To ihnatus

Fixed. Also, added a notice on stdout (not shown to the players) if the gold is underflown.

Clarification: This is freeciv logging (like it should), and not necessarily stdout.

2022-06-16 13:29 Updated by: cazfi
  • Status Update from Open to Closed
  • Resolution Update from Accepted to Fixed

Attachment File List

Edit

Please login to add comment to this ticket » Login