bugFreeciv - Bugs: bug #20495, punit->activity_count overflow

 
 
Show feedback again

bug #20495: punit->activity_count overflow

Submitted by:  pepeto <pepeto>
Submitted on:  Sun Feb 10 16:25:46 2013  
 
Category: generalSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Jacob Nevins <jtn>Open/Closed: Closed
Release: 2.3.4,2.4.0-beta1Operating System: Any
Planned Release: 2.3.5,2.4.0,2.5.0Contains string changes: None

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Tue Mar 26 02:03:14 2013, comment #9:

Another thing worth noting about this patch: for completeness, it widens the PACKET_EDIT_UNIT.activity_count field as well as the ones in PACKET_UNIT_INFO, but that field is neither read nor written currently (in common with several other fields in EDIT_UNIT).

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Tue Mar 26 02:00:57 2013, SVN revision 22605:

Increase size of activity_count fields over the network to 16 bits, to
prevent overflow with long activities such as transforming to/from ocean.

Reported by pepeto@gna.

See gna bug #20495.

(Browse SVN revision 22605)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Tue Mar 26 02:00:19 2013, SVN revision 22603:

Increase size of activity_count fields over the network to 16 bits, to
prevent overflow with long activities such as transforming to/from ocean.

Reported by pepeto@gna.

See gna bug #20495.

(Browse SVN revision 22603)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Tue Mar 26 01:51:36 2013, SVN revision 22601:

Increase size of activity_count fields over the network to 16 bits, to
prevent overflow with long activities such as transforming to/from ocean.
Add checks to rulesets for compatibility with the expanded limit.

Reported by pepeto@gna.

See gna bug #20495.

(Browse SVN revision 22601)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sun Mar 24 00:50:37 2013, comment #5:

Attached patches for all branches which increase the size of the network field to UINT16.

The trunk version adds additional checks to ruleset loading to ensure activities defined in rulesets don't go over the (now expanded) limit.

(file #17528, file #17529, file #17530)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sat Mar 23 22:21:51 2013, comment #4:

I've split out the (cosmetic) sentry issue to bug #20641 (even though it was the original issue of this ticket), and kept this one for dealing with the overflow.

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sat Mar 23 22:00:27 2013, comment #3:

The patch attached seems like a fine thing to apply.
The consequences of the bug for ACTIVITY_SENTRY seem entirely harmless: the client uses activity_count to display how many turns are left on an activity, but of course not for ACTIVITY_SENTRY. So I think we get away with it.

> However, what should be done when loading savegames with such
> huge activity counts?

The obvious thing would be to apply the same check as in update_unit_activity() and zero incoming activity_count for activities where it makes no sense.
However, that means duplicating logic. We could just put in a special case for ACTIVITY_SENTRY, to quell these warnings from the network code.

> It also affects Engineers when doing long job (e.g.
> transforming swamp to ocean in default ruleset) in all
> branches.

...because of ACTIVITY_FACTOR=10 * transform_time=36: total required activity will be 360 points, easily overflowing the uint8. Oh dear.

The consequence is that the time left displayed in the client will suddenly increase, but the transformation will complete on schedule. Verified in testing: a single Engineer transforming to ocean apparently jumps from 6 turns left to 18 turns left.
I guess most people won't see this because they'll use gangs of Engineers for such a project, and no individual unit will accrue enough points to trigger an overflow.

The obvious solution is to increase these fields to a uint16 (using an optional capability on stable branches). I don't think this should lead to a large increase in network traffic, as I think delta compression means that the extra octet will only be transmitted once a turn, or whenever a unit changes activity?

(I briefly toyed with not scaling by ACTIVITY_FACTOR over the network, but I don't think that works.)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sat Feb 23 09:37:25 2013, comment #2:

It also affects Engineers when doing long job (e.g. transforming swamp to ocean in default ruleset) in all branches.

pepeto <pepeto>
Project Member
Tue Feb 19 11:39:55 2013, comment #1:

> Noticed that units fortifying have a increasing activity count.


I was wrong. The activity of the units with strange activity count were sentry.

I found at r5333 that ACTIVITY_SENTRY has been forgotten as activity "where activity count is
irrelevant". Or may be that count is relevant? I don't think so. The attached patch should fix that count.

However, what should be done when loading savegames with such huge activity counts?

Also, why do these AI Muskets and Pikemen are sentry? What advantage do they get? I think they should be fortified, shouldn't they?

(file #17263)

pepeto <pepeto>
Project Member
Sun Feb 10 16:25:46 2013, original submission:

Noticed that units fortifying have a increasing activity count. You can read in the savegame posted in bug #20361 pikemen with a counter of 1200!

The problem is that this info is sent to the clients using a 8-bit integer...

pepeto <pepeto>
Project Member

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by jtn (Posted a comment)
  • -unavailable- added by pepeto (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Error: not logged in

     

     

    Follow 13 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue Mar 26 02:03:14 2013jtnStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Sun Mar 24 00:50:37 2013jtnAttached File-=>Added trunk-activity-count-range.patch, #17528
      Attached File-=>Added S2_4-activity-count-range.patch, #17529
      Attached File-=>Added S2_3-activity-count-range.patch, #17530
      StatusIn Progress=>Ready For Test
    Sat Mar 23 22:21:51 2013jtnStatusNeed Info=>In Progress
      Assigned toNone=>jtn
      Release=>2.3.4,2.4.0-beta1
      Operating SystemNone=>Any
      Planned Release=>2.3.5,2.4.0,2.5.0
    Tue Feb 19 11:39:55 2013pepetoAttached File-=>Added update_unit_activity_sentry.diff, #17263
      StatusNone=>Need Info
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup