Anonymous Login
2017-11-21 15:02 CET

View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0001389OpenClonkEngine - C4Scriptpublic2017-10-26 18:52
ReporterIsilkor 
Assigned ToGünther 
PrioritynormalSeverityminorReproducibilityalways
StatusresolvedResolutionopen 
Product Version 
Target Versiongit masterFixed in Version 
Summary0001389: Aul integer overflow results in strange numbers on 64 bit machines
DescriptionAul currently uses signed 32 bit integers to represent numbers. However, because it's storing its value inside an intptr_t variable, it is possible to create C4Values which have their internal representation sign-extended to 64 bit. Such numbers don't necessarily compare equal to other numbers that appear to be the same (but were zero-extended internally).
Steps To Reproduce-> 2147483647 + 1
 = -2147483648
-> 2147483647 + 1 == -2147483648
 = false

-> -2147483648 - 1
 = 2147483647
-> -2147483648 - 1 == 2147483647
 = false
Additional InformationSigned overflow is undefined behavior according to the C and C++ specs.
TagsNo tags attached.
Attached Files

-Relationships
+Relationships

-Notes

~0003787

Isilkor (developer)

The reason for this is C4Value::operator++ (and operator--), which are operating on the internal intptr_t value directly. C4AulParse reduces x + 1 and x - 1 to increment and decrement respectively, which use the C4Value operators.

~0003788

Caesar (developer)

Well, integer overflow is ub… I don't see a problem here.

~0003789

Isilkor (developer)

The problem is that C4Script is not C.

~0004276

Sven2 (developer)

But if we zero the high DWORD after every arithmetic operation, we should be fine?

~0004277

Sven2 (developer)

No wait, we can just let ++ and -- work only on the int32_t?

~0004285

Isilkor (developer)

There is no int32_t, the integer value is stored as an intptr_t (because we need all union members to be the same size because the comparison operator is using undefined behaviour that just happens to work at the moment if we do it like this). We should either use setInt, which does the right thing by zero-extending the int32_t to an intptr_t before storing, or zero-extend in operator++ and operator--.

(We should also cast all arithmetic operations to uint32_t before doing them, because signed overflow is undefined.)

~0004286

Caesar (developer)

Any reason we cannot just overload the operator to be defined behaviour?

~0004287

Isilkor (developer)

My reason is mostly that it's easier to have to do something in just one place than in all of them. (And good luck finding all of them.)

~0004289

Sven2 (developer)

Why are there many places? Shouldn't the internal 64 bit field be private and C4Value::GetInt()/SetInt() used for all integer operations?

~0004295

Isilkor (developer)

Oh this is more of a general thing, since evidently the reason for this bug is the existence of the operators. Had we used GetInt()/SetInt() in the first place, this wouldn't have happened.

~0005849

Clonkonaut (developer)

Detached target version. Don't know how serious this is or if it will get fixed.

~0005861

Isilkor (developer)

Seems to have been fixed in 21d8a2a64ad5ed8150353bbe40a5b19571796ab1
+Notes

-Issue History
Date Modified Username Field Change
2015-09-07 19:15 Isilkor New Issue
2015-09-07 19:21 Isilkor Steps to Reproduce Updated View Revisions
2015-09-07 19:57 Isilkor Note Added: 0003787
2015-09-08 04:08 Caesar Note Added: 0003788
2015-09-08 10:34 Isilkor Note Added: 0003789
2015-10-16 02:33 Sven2 Target Version => 7.0
2015-12-06 23:52 Clonkonaut Target Version 7.0 => 7.1 (Bugfix update)
2015-12-07 02:51 Sven2 Note Added: 0004276
2015-12-07 02:53 Sven2 Note Added: 0004277
2015-12-07 13:32 Isilkor Note Added: 0004285
2015-12-07 16:26 Caesar Note Added: 0004286
2015-12-07 16:27 Isilkor Note Added: 0004287
2015-12-07 21:43 Sven2 Note Added: 0004289
2015-12-08 21:31 Isilkor Note Added: 0004295
2017-08-05 11:06 Maikel Target Version 7.1 (Bugfix update) => 8.0
2017-10-26 15:30 Clonkonaut Note Added: 0005849
2017-10-26 15:31 Clonkonaut Target Version 8.0 => git master
2017-10-26 18:52 Isilkor Assigned To => Günther
2017-10-26 18:52 Isilkor Status new => resolved
2017-10-26 18:52 Isilkor Note Added: 0005861
+Issue History