bugFreeciv - Bugs: bug #15426, BASE type should be signed

 
 
Show feedback again

bug #15426: BASE type should be signed

Submitted by:  None
Submitted on:  Fri 19 Feb 2010 04:50:56 AM UTC  
 
Category: generalSeverity: 4 - Important
Priority: 5 - NormalStatus: Fixed
Assigned to: pepeto <pepeto>Originator Email: -unavailable-
Open/Closed: ClosedRelease: 
Operating System: NonePlanned Release: 2.2.1

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)

Thu 25 Feb 2010 07:55:57 PM UTC, SVN revision 16932:

BASE type is now signed to handle BASE_NONE (-1) correctly.
As this doesn't change the structure of the packets and this value wasn't used
for values > 127 (31 was the max), the capability string is not updated.

Reported by -unavailable-

See gna bug #15426

(Browse SVN revision 16932)

pepeto <pepeto>
Project MemberIn charge of this item.
Thu 25 Feb 2010 07:55:53 PM UTC, SVN revision 16931:

BASE type is now signed to handle BASE_NONE (-1) correctly.
As this doesn't change the structure of the packets and this value wasn't used
for values > 127 (31 was the max), the capability string is not updated.

Reported by -unavailable-

See gna bug #15426

(Browse SVN revision 16931)

pepeto <pepeto>
Project MemberIn charge of this item.
Wed 24 Feb 2010 07:44:03 AM UTC, comment #5:

> For S2_2: Could someone affirm that changing a type uint8 to
> sint8 is safe or not in the network code? This field contains
> only small values [-1; 32[.


After a brunch of tests, I am now certain that changing this type won't break the compatibility, as values over 127 are not used. So no need to perform a capability string update in trunk, and use the trunk patch on S2_2 branch... Ready for test!

pepeto <pepeto>
Project MemberIn charge of this item.
Tue 23 Feb 2010 09:30:56 PM UTC, comment #4:

Attached a version for trunk, which would probably a capability string change.

For S2_2: Could someone affirm that changing a type uint8 to sint8 is safe or not in the network code? This field contains only small values [-1; 32[.
Another solution would to define tons of pre-send and post-send function, but if a such noise could be avoided.

(file #8241)

pepeto <pepeto>
Project MemberIn charge of this item.
Tue 23 Feb 2010 08:59:44 PM UTC, comment #3:

Right point.

pepeto <pepeto>
Project MemberIn charge of this item.
Tue 23 Feb 2010 08:54:18 PM UTC, comment #2:

This bug can potentially cause the server to crash. For example, consider server/unittools.c : 744

if (what_pillaged == S_LAST && punit->activity_base != -1) {
unit_pillage_base(ptile, base_by_number(punit->activity_base));
} else {
tile_clear_special(ptile, what_pillaged);
}

Note that the field activity_base has type Base_type_id. If it ends up with the value 255 instead of -1 because of the bug, the if-clause will execute, calling unit_pillage_base with NULL as the base. The unit_pillage_base function dereferences the NULL pointer. Game over.

Is there any objection to the proposed fix? It seems clear that if a type uses signed values, it should be a signed type.

Anonymous
Sat 20 Feb 2010 10:29:57 AM UTC, comment #1:

I checked how it is handled by the client and server. -1 to 255 doesn't seems have critical issues. However, it is not really clean...

pepeto <pepeto>
Project MemberIn charge of this item.
Fri 19 Feb 2010 04:50:56 AM UTC, original submission:

At revision 15190, the type "enum base_type_id" was replaced with the type "Base_type_id", which is an int. When this change was made, occurrences of the enum value BASE_LAST were changed to the literal integer value "-1." However, in the network code (common/packets.def), the associated network type was defined as

type BASE = uint8(Base_type_id)

Note that is an unsigned type. Hence when the value -1 is sent as a Base_type_id (which happens frequently), it is incorrectly received on the other end as the value 255.

I think the right thing to do here is change the definition of BASE to the corresponding 8-bit signed type.

type BASE = sint8(Base_type_id)

An additional issue may be that the literal value -1 should be replaced with a symbolic constant such as BASE_TYPE_INVALID or BASE_NONE in the code. But even if that is a valid issue, it is separate from this issue, which is about how the value 0xff is interpreted when received.

JKL

Anonymous

 

(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 pepeto (Posted a comment)
  • -unavailable- added by None (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 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 25 Feb 2010 07:56:12 PM UTCpepetoStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Wed 24 Feb 2010 07:44:03 AM UTCpepetoStatusIn Progress=>Ready For Test
      Assigned toNone=>pepeto
    Tue 23 Feb 2010 09:30:56 PM UTCpepetoAttached File-=>Added trunk_signed_BASE_type.diff, #8241
      StatusNone=>In Progress
    Tue 23 Feb 2010 08:59:44 PM UTCpepetoSeverity3 - Normal=>4 - Important
      Planned Release=>2.2.1
    Sat 20 Feb 2010 10:29:57 AM UTCpepetoCategoryNone=>general
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup