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

 
 
Show feedback again

bug #20495: punit->activity_count overflow

Submitted by:  pepeto <pepeto>
Submitted on:  Sun 10 Feb 2013 04:25:46 PM UTC  
 
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.0

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 26 Mar 2013 02:03:14 AM UTC, 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 26 Mar 2013 02:00:57 AM UTC, 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 26 Mar 2013 02:00:19 AM UTC, 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 26 Mar 2013 01:51:36 AM UTC, 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 24 Mar 2013 12:50:37 AM UTC, 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 23 Mar 2013 10:21:51 PM UTC, 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 23 Mar 2013 10:00:27 PM UTC, 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 23 Feb 2013 09:37:25 AM UTC, 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 19 Feb 2013 11:39:55 AM UTC, 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 10 Feb 2013 04:25:46 PM UTC, 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.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 13 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 26 Mar 2013 02:03:14 AM UTCjtnStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Sun 24 Mar 2013 12:50:37 AM UTCjtnAttached 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 23 Mar 2013 10:21:51 PM UTCjtnStatusNeed 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 19 Feb 2013 11:39:55 AM UTCpepetoAttached File-=>Added update_unit_activity_sentry.diff, #17263
      StatusNone=>Need Info
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup