player1 suicides when stopping while walking up stairs

[sauerbraten]

apparently, just like that. with no user input... (?)

reported by Shag as well as Eleisa.

[sauerbraten]

video of the bug provided by Eleisa: https://streamable.com/04vb7n a demo of it happening would be very helpful!

[sauerbraten]

p1xbraten_suicide_bug_xion.7.35.dmo.zip

[sauerbraten]

the demo shows a position packet with a Z position value > 0xFFFFFF (more precisely, a value in 0x[00-FF]000000) that is broadcasted by the server as three zero bytes, followed by a jumppad packet for the player (in this case, with ent ID 1, one of the jumppads on reissen).

this suggests a call chain along the lines of checkitems() -> trypickup() -> jumppadeffects(). however, I can't explain how the distance check in checkitems() passes, when the jumppad with ent ID 1 is at Z=~1290. (the affected player is at Z=~1424 in the demo, and not really close in terms of X and Y at all.) the suicide is then most likely triggered by the 0 Z value of the player position together with reissen's death floor.

[sauerbraten]

the last "known good" position packet of the bugging player is: 4 0 13 0 250 76 190 52 232 88 162 109 90 0 196 127

in this packet, the position's Z component is encoded as 232 88, which decoded comes out to 1422.5 (22760 on the wire, before downscaling). ((232 + 88<<8) / 16.0, 16.0 is the scaling factor for world locations.)

the same packet occurs 110 times unchanged, followed by: 4 0 9 4 250 76 190 52 0 0 0 162 109 90 0 196 127

• the change from 13 to 9 in the third byte means a change in physent::physstate from PHYS_STEP_UP to PHYS_FALLING (only the 3 lowest bits are actually physstate).
• the change from 0 to 4 in the fourth byte only indicates that the Z component of the position vector sent later in the packet will take up 3 bytes instead of 2 (which are enough most of the time).
• the last change then is in the Z component, from 232 88 to 0 0 0. the code in fpsgame/client.cpp::sendposition() indicates that the 3 byte long form is chosen when the Z component of the integerized position vector is either < 0 or > 0xFFFF (65535). since the 24 lowest bits are all 0, this leaves the aforementioned range of 0x[00-FF]000000 for the z value.

in order for jumppadeffects() to be involved in this bug, the Z component has to be kinda close to the jumppad's when the distance is calculated inside checkitems(), and then be super high in jumppadeffects() where sendposition() is called. or the distance math suffers from an overflow of some sort.

also, notably, while jumppadeffects() does set physstate = PHYS_FALLING, it happens after the position was already sent, so this doesn't seem to be an explanation for the change in the third byte.

[sauerbraten]

another occurence: p1xbraten_suicide_bug_eleisa.9.20.zip

parsed with https://github.com/sauerbraten/dmolisher:

gamemillis, channel, data length, data (bytes in decimal)
46623, 0, 32, [4 2 5 0 90 33 194 32 129 36 157 102 90 0 146 126 4 0 4 0 238 42 16 17 225 37 86 105 90 0 152 127]
46623, 1,  5, [88 2 2 32 9]
46644, 0, 17, [4 2 1 4 90 33 194 32 0 0 0 157 102 90 0 146 126]
46644, 0,  3, [29 2 0]
46644, 0, 18, [4 2 1 12 90 33 194 32 0 0 0 157 102 90 144 1 32 253]
46644, 0,  4, [28 2 3 4]
46644, 0, 17, [4 2 1 8 127 56 0 29 255 35 153 101 90 144 1 32 253]
46644, 0,  3, [29 2 6]
46656, 0, 34, [4 2 1 24 127 56 0 29 37 36 153 101 90 38 1 32 253 2 4 0 4 0 238 42 16 17 225 37 86 105 90 0 152 127]
46661, 1,  5, [11 2 2 255 0]

same as in the first demo from reissen:

• physstate changes from PHYS_STEP_UP to PHYS_FALLING
• the "Z uses three bytes" flag is set
• Z gets transmitted as 0 0 0
• no other change in the position packet

from then, it's different:

• jumppad 2 is activated
• in the position packet after that, both the Z component and the velocity vector take up an extra byte:
  • position is unchanged, Z is still transmitted as 0 0 0
  • velocity magnitude is 400 (144 + 1<<8)
  • velocity direction is changed, but I don't think it's value is relevant
• teleport 3 (with teledest 4) is activated
• in the next position packet, the "Z takes 3 bytes" flag is unset:
  • position changes (probably to the teledest position), Z = 576
  • camera1 direction changes (likely to the teledest direction)
  • velocity direction and magnitude are unchanged
• jumppad 6 is activated
• in the next position packet, the "fall magnitude present" flag is set:
  • position Z changes slightly, to 578
  • fall magnitude is 2

[sauerbraten]

the sequence in the second recorded case fits the theory that it's checkitems() going haywire, since the ents are all trypickup()ed in ascending order (2, 3, 6) and basically at the same time.

[sauerbraten]

Eleisa confirmed for me that it's reproducible (on macOS) by walking up stairs and stopping on them, so physstate == PHYS_STEP_UP is required and standing still while in that state seems to trigger the bug (~5 seconds after releasing all inputs).

[sauerbraten]

debug prints in checkitems()/trypickup() show that the entity's position's Y component as well as the feetposition's Z component get out of whack and are printed as "nan" by floatstr().

[sauerbraten]

the bug seems to only occur when building with clang and -O1/-O2/-O3. building the object files with gcc 11 and -O3, then linking in the final step using clang (for -f framework includes to work) produces a working binary.

[sauerbraten]

sometimes, instead of activating far-away ents, the hudgun disappears.

[sauerbraten]

building with -O3 and without -ffast-math also works fine. it seems somewhere the player's Z becomes NaN, and that trips up things like the distance function.

[sauerbraten]

the source of the bug seems to be that in this line https://github.com/sauerbraten/p1xbraten/blob/9c7ac12502d375d20e6e8d68f87a8f9ecb72808a/src/engine/physics.cpp#L1177 the expression dir.magnitude() sometimes, when dir is (0, -0, 0) returns -Infinity instead of the expected -0/+0. magnitude() is defined as https://github.com/sauerbraten/p1xbraten/blob/9c7ac12502d375d20e6e8d68f87a8f9ecb72808a/src/shared/geom.h#L131 and dir.squaredlen() still returns a correct 0.000000. something is weird with sqrtf() when clang uses -ffast-math and optimizations.

[sauerbraten]

the fix for now will be to build using gcc, which is also how the official binaries are built.