isledecomp/isle-portable
1
0
Fork 0
mirror of https://github.com/isledecomp/isle-portable.git synced 2026-05-01 18:13:57 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Fix use-after-free crash and misaligned protocol access

Browse Source 
Stop active ScenePlayer animation in OnActorEnter/OnActorExit before
modifying ride animation state — the ScenePlayer may still hold a
reference to the ride vehicle ROI that ClearRideAnimation frees.
Deactivate() and OnWorldDisabled() already had this guard.

Add alignment padding to MessageHeader (13→14 bytes) so uint16_t fields
in packed protocol structs no longer sit at odd offsets (UBSan violation).
Breaking wire format change — all clients and relay must update together.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Christian Semmler 2026-03-28 11:04:06 -07:00
parent f3c6d90cf1
commit e57164d345
No known key found for this signature in database
GPG Key ID: 086DAA1360BEEE5C
5 changed files with 40 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
extensions/include/extensions/multiplayer/protocol.h
View File

@ -78,6 +78,7 @@ enum LightChangeType : uint8_t {
struct MessageHeader {
uint8_t type;
uint8_t _pad;
uint32_t peerId;
uint32_t sequence;
uint32_t target;

18
extensions/src/multiplayer/networkmanager.cpp
View File

@ -175,7 +175,7 @@ MxResult NetworkManager::Tickle()
}
else if (IsConnected()) {
AnimCancelMsg cancelMsg{};
cancelMsg.header = {MSG_ANIM_CANCEL, m_localPeerId, m_sequence++, TARGET_HOST};
cancelMsg.header = {MSG_ANIM_CANCEL, 0, m_localPeerId, m_sequence++, TARGET_HOST};
SendMessage(cancelMsg);
}
m_localPendingAnimInterest = -1;
@ -326,7 +326,7 @@ void NetworkManager::CancelLocalAnimInterest()
}
else if (IsConnected()) {
AnimCancelMsg msg{};
msg.header = {MSG_ANIM_CANCEL, m_localPeerId, m_sequence++, TARGET_HOST};
msg.header = {MSG_ANIM_CANCEL, 0, m_localPeerId, m_sequence++, TARGET_HOST};
SendMessage(msg);
}
@ -638,7 +638,7 @@ void NetworkManager::ProcessPendingRequests()
}
else if (IsConnected()) {
AnimInterestMsg msg{};
msg.header = {MSG_ANIM_INTEREST, m_localPeerId, m_sequence++, TARGET_HOST};
msg.header = {MSG_ANIM_INTEREST, 0, m_localPeerId, m_sequence++, TARGET_HOST};
msg.animIndex = animIndex;
ThirdPersonCamera::Controller* animCam = GetCamera();
msg.displayActorIndex = animCam ? animCam->GetDisplayActorIndex() : 0;
@ -715,7 +715,7 @@ void NetworkManager::BroadcastLocalState()
}
PlayerStateMsg msg{};
msg.header = {MSG_STATE, m_localPeerId, m_sequence++, TARGET_BROADCAST};
msg.header = {MSG_STATE, 0, m_localPeerId, m_sequence++, TARGET_BROADCAST};
msg.actorId = actorId;
msg.worldId = inRestrictedArea ? WORLD_NOT_VISIBLE : (int8_t) currentWorld->GetWorldId();
msg.vehicleType = DetectVehicleType(userActor);
@ -1066,7 +1066,7 @@ void NetworkManager::SendEmote(uint8_t p_emoteId)
cam->TriggerEmote(p_emoteId);
EmoteMsg msg{};
msg.header = {MSG_EMOTE, m_localPeerId, m_sequence++, TARGET_BROADCAST};
msg.header = {MSG_EMOTE, 0, m_localPeerId, m_sequence++, TARGET_BROADCAST};
msg.emoteId = p_emoteId;
SendMessage(msg);
}
@ -1201,7 +1201,7 @@ bool NetworkManager::IsClonedCharacter(const char* p_name) const
void NetworkManager::SendCustomize(uint32_t p_targetPeerId, uint8_t p_changeType, uint8_t p_partIndex)
{
CustomizeMsg msg{};
msg.header = {MSG_CUSTOMIZE, m_localPeerId, m_sequence++, TARGET_BROADCAST_ALL};
msg.header = {MSG_CUSTOMIZE, 0, m_localPeerId, m_sequence++, TARGET_BROADCAST_ALL};
msg.targetPeerId = p_targetPeerId;
msg.changeType = p_changeType;
msg.partIndex = p_partIndex;
@ -1664,7 +1664,7 @@ void NetworkManager::HandleAnimStartLocally(uint16_t p_animIndex, bool p_localIn
AnimUpdateMsg NetworkManager::BuildAnimUpdateMsg(uint16_t p_animIndex, uint32_t p_target)
{
AnimUpdateMsg msg{};
msg.header = {MSG_ANIM_UPDATE, m_localPeerId, m_sequence++, p_target};
msg.header = {MSG_ANIM_UPDATE, 0, m_localPeerId, m_sequence++, p_target};
msg.animIndex = p_animIndex;
const Animation::AnimSession* session = m_animSessionHost.FindSession(p_animIndex);
@ -1699,7 +1699,7 @@ void NetworkManager::SendAnimUpdateToPlayer(uint16_t p_animIndex, uint32_t p_tar
void NetworkManager::BroadcastAnimStart(uint16_t p_animIndex)
{
AnimStartMsg msg{};
msg.header = {MSG_ANIM_START, m_localPeerId, m_sequence++, TARGET_BROADCAST};
msg.header = {MSG_ANIM_START, 0, m_localPeerId, m_sequence++, TARGET_BROADCAST};
msg.animIndex = p_animIndex;
SendMessage(msg);
@ -1720,7 +1720,7 @@ void NetworkManager::BroadcastAnimComplete(uint16_t p_animIndex)
}
AnimCompleteMsg msg{};
msg.header = {MSG_ANIM_COMPLETE, m_localPeerId, m_sequence++, TARGET_BROADCAST};
msg.header = {MSG_ANIM_COMPLETE, 0, m_localPeerId, m_sequence++, TARGET_BROADCAST};
msg.eventId = (static_cast<uint64_t>(SDL_rand_bits()) << 32) | static_cast<uint64_t>(SDL_rand_bits());
msg.objectId = animInfo->m_objectId;
msg.participantCount = 0;

12
extensions/src/multiplayer/server/protocol.ts
View File

@ -1,10 +1,10 @@
// Protocol constants — must stay in sync with protocol.h
// MessageHeader binary layout: type(1) + peerId(4) + sequence(4) + target(4) = 13 bytes
export const HEADER_SIZE = 13;
export const PEER_ID_OFFSET = 1;
export const SEQUENCE_OFFSET = 5;
export const TARGET_OFFSET = 9;
// MessageHeader binary layout: type(1) + _pad(1) + peerId(4) + sequence(4) + target(4) = 14 bytes
export const HEADER_SIZE = 14;
export const PEER_ID_OFFSET = 2;
export const SEQUENCE_OFFSET = 6;
export const TARGET_OFFSET = 10;
// Routing target constants
export const TARGET_BROADCAST = 0;
@ -19,7 +19,7 @@ export const MSG_ASSIGN_ID = 0xff;
// AssignIdMsg: compact server-only message — type(1) + peerId(4)
const ASSIGN_ID_SIZE = 1 + 4;
// HostAssignMsg: header(13) + hostPeerId(4)
// HostAssignMsg: header(14) + hostPeerId(4)
const HOST_ASSIGN_SIZE = HEADER_SIZE + 4;
export function createAssignIdMsg(peerId: number): ArrayBuffer {

8
extensions/src/multiplayer/worldstatesync.cpp
View File

@ -237,7 +237,7 @@ MxBool WorldStateSync::HandleSkyLightMutation(uint8_t p_entityType, uint8_t p_ch
void WorldStateSync::SendSnapshotRequest()
{
RequestSnapshotMsg msg{};
msg.header = {MSG_REQUEST_SNAPSHOT, m_localPeerId, m_sequence++, TARGET_HOST};
msg.header = {MSG_REQUEST_SNAPSHOT, 0, m_localPeerId, m_sequence++, TARGET_HOST};
SendMessage(msg);
m_snapshotRequested = true;
@ -275,7 +275,7 @@ void WorldStateSync::SendWorldSnapshot(uint32_t p_targetPeerId)
stateBuffer[dataLength++] = (uint8_t) lightPos;
WorldSnapshotMsg msg{};
msg.header = {MSG_WORLD_SNAPSHOT, m_localPeerId, m_sequence++, p_targetPeerId};
msg.header = {MSG_WORLD_SNAPSHOT, 0, m_localPeerId, m_sequence++, p_targetPeerId};
msg.dataLength = (uint16_t) dataLength;
std::vector<uint8_t> msgBuf(sizeof(WorldSnapshotMsg) + dataLength);
@ -288,7 +288,7 @@ void WorldStateSync::SendWorldSnapshot(uint32_t p_targetPeerId)
void WorldStateSync::BroadcastWorldEvent(uint8_t p_entityType, uint8_t p_changeType, uint8_t p_entityIndex)
{
WorldEventMsg msg{};
msg.header = {MSG_WORLD_EVENT, m_localPeerId, m_sequence++, TARGET_BROADCAST};
msg.header = {MSG_WORLD_EVENT, 0, m_localPeerId, m_sequence++, TARGET_BROADCAST};
msg.entityType = p_entityType;
msg.changeType = p_changeType;
msg.entityIndex = p_entityIndex;
@ -298,7 +298,7 @@ void WorldStateSync::BroadcastWorldEvent(uint8_t p_entityType, uint8_t p_changeT
void WorldStateSync::SendWorldEventRequest(uint8_t p_entityType, uint8_t p_changeType, uint8_t p_entityIndex)
{
WorldEventRequestMsg msg{};
msg.header = {MSG_WORLD_EVENT_REQUEST, m_localPeerId, m_sequence++, TARGET_HOST};
msg.header = {MSG_WORLD_EVENT_REQUEST, 0, m_localPeerId, m_sequence++, TARGET_HOST};
msg.entityType = p_entityType;
msg.changeType = p_changeType;
msg.entityIndex = p_entityIndex;

20
extensions/src/thirdpersoncamera/controller.cpp
View File

@ -105,6 +105,16 @@ void Controller::OnActorEnter(IslePathActor* p_actor)
return;
}
// Stop external animation before modifying ride/display state —
// the ScenePlayer may hold a reference to the ride vehicle ROI.
if (m_animPlaying) {
if (m_animStopCallback) {
m_animStopCallback();
}
m_animPlaying = false;
m_animStopCallback = nullptr;
}
LegoROI* newROI = userActor->GetROI();
if (!newROI) {
return;
@ -157,6 +167,16 @@ void Controller::OnActorExit(IslePathActor* p_actor)
return;
}
// Stop external animation before clearing ride animation state —
// the ScenePlayer may hold a reference to the ride vehicle ROI.
if (m_animPlaying) {
if (m_animStopCallback) {
m_animStopCallback();
}
m_animPlaying = false;
m_animStopCallback = nullptr;
}
if (m_animator.GetCurrentVehicleType() != VEHICLE_NONE) {
m_animator.ClearRideAnimation();
m_animator.ClearAll();