| View Issue Details [ Jump to Notes ] | [ Issue History ] [ Print ] | ||||||||
| ID | Project | Category | View Status | Date Submitted | Last Update | ||||
|---|---|---|---|---|---|---|---|---|---|
| 0001389 | OpenClonk | Engine - C4Script | public | 2015-09-07 17:15 | 2017-10-26 16:52 | ||||
| Reporter | Isilkor | ||||||||
| Assigned To | Günther | ||||||||
| Priority | normal | Severity | minor | Reproducibility | always | ||||
| Status | resolved | Resolution | open | ||||||
| Product Version | |||||||||
| Target Version | git master | Fixed in Version | |||||||
| Summary | 0001389: Aul integer overflow results in strange numbers on 64 bit machines | ||||||||
| Description | Aul 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 Information | Signed overflow is undefined behavior according to the C and C++ specs. | ||||||||
| Tags | No tags attached. | ||||||||
| Attached Files |
| ||||||||
 Relationships |
|
 Relationships |
| Notes |
|
|
Isilkor (developer) 2015-09-07 17:57 |
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) 2015-09-08 02:08 |
Well, integer overflow is ub… I don't see a problem here. |
|
Isilkor (developer) 2015-09-08 08:34 |
The problem is that C4Script is not C. |
|
Sven2 (developer) 2015-12-07 01:51 |
But if we zero the high DWORD after every arithmetic operation, we should be fine? |
|
Sven2 (developer) 2015-12-07 01:53 |
No wait, we can just let ++ and -- work only on the int32_t? |
|
Isilkor (developer) 2015-12-07 12:32 |
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) 2015-12-07 15:26 |
Any reason we cannot just overload the operator to be defined behaviour? |
|
Isilkor (developer) 2015-12-07 15:27 |
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) 2015-12-07 20:43 |
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) 2015-12-08 20:31 |
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) 2017-10-26 13:30 |
Detached target version. Don't know how serious this is or if it will get fixed. |
|
Isilkor (developer) 2017-10-26 16:52 |
Seems to have been fixed in 21d8a2a64ad5ed8150353bbe40a5b19571796ab1 |
| Notes |
| 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 |
