Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: transfer house crash #3104

Merged
merged 1 commit into from
Nov 13, 2024
Merged

fix: transfer house crash #3104

merged 1 commit into from
Nov 13, 2024

Conversation

dudantas
Copy link
Contributor

@dudantas dudantas commented Nov 12, 2024

Description

This addresses an issue in the House::resetTransferItem function where the transferItem pointer was being set to nullptr before being used later in the function, leading to potential undefined behavior or crashes. The solution involves reordering the operations to ensure that transferItem is only set to nullptr after all required operations are completed. This change ensures that the object remains accessible until all necessary manipulations are performed.

Fixes this crash: crash house.txt

Behaviour

Actual

When transferItem is set to nullptr before the removal of the item, attempting to use the tmpItem reference leads to undefined behavior or a crash because the reference is pointing to a nullptr.

Expected

transferItem should only be set to nullptr after tmpItem is fully utilized to remove the item from transfer_container. This avoids dereferencing a nullptr and ensures that operations on the item are safe.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested

The change was tested by:

  • Validating that the resetTransferItem function works without setting transferItem to nullptr before necessary operations are performed.

  • Running the server under conditions that would trigger the resetTransferItem function to ensure there are no crashes or unexpected behavior.

  • Adding logs to verify that tmpItem retains its value correctly until all actions are completed.

  • Test A: Triggered resetTransferItem to confirm correct handling of the item reference without setting transferItem to nullptr prematurely.

  • Test B: Stress-tested the scenario where multiple items are transferred in quick succession to validate stability.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I checked the PR checks reports
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works

Copy link

sonarcloud bot commented Nov 12, 2024

@dudantas dudantas merged commit 6239786 into main Nov 13, 2024
35 checks passed
@dudantas dudantas deleted the dudantas/fix-house-transfer-crash branch November 13, 2024 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants