Anonymous Login
2021-09-24 15:55 UTC

View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0001389OpenClonkEngine - C4Scriptpublic2017-10-26 16:52
Assigned ToGünther 
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




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.


Caesar (developer)

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


Isilkor (developer)

The problem is that C4Script is not C.


Sven2 (developer)

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


Sven2 (developer)

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


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.)


Caesar (developer)

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


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.)


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?


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.


Clonkonaut (developer)

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


Isilkor (developer)

Seems to have been fixed in 21d8a2a64ad5ed8150353bbe40a5b19571796ab1

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