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

Jump to bottom

Heap use after free when oil rig is removed #7820

Closed
JGRennison opened this issue Nov 2, 2019 · 0 comments
Closed

Heap use after free when oil rig is removed #7820

JGRennison opened this issue Nov 2, 2019 · 0 comments

Comments

@JGRennison
Copy link
Contributor

Version of OpenTTD

Since f538179

Expected result

No heap use after free when oil rig is removed (either using demolish tool or industry self-closure).

Actual result

Heap use after free when oil rig is removed (either using demolish tool or industry self-closure).

AddressSanitizer output:

==26438==ERROR: AddressSanitizer: heap-use-after-free on address 0x6250001ae1b8 at pc 0x55d8d8c795b4 bp 0x7ffe765c3370 sp 0x7ffe765c3360
READ of size 4 at 0x6250001ae1b8 thread T0
    #0 0x55d8d8c795b3 in OrthogonalTileArea::Add(unsigned int) /home/jgr/openttd/trunk4/src/tilearea.cpp:47
    #1 0x55d8d9024b11 in CheckForDockingTile(unsigned int) /home/jgr/openttd/trunk4/src/water_cmd.cpp:197
    #2 0x55d8d9026a3a in MakeWaterKeepingClass(unsigned int, Owner) /home/jgr/openttd/trunk4/src/water_cmd.cpp:252
    #3 0x55d8d781126c in Industry::~Industry() /home/jgr/openttd/trunk4/src/industry_cmd.cpp:155
    #4 0x55d8d7816ca7 in ClearTile_Industry /home/jgr/openttd/trunk4/src/industry_cmd.cpp:516
    #5 0x55d8d78c02f5 in CmdLandscapeClear(unsigned int, DoCommandFlag, unsigned int, unsigned int, char const*) /home/jgr/openttd/trunk4/src/landscape.cpp:724
    #6 0x55d8d729aeb6 in DoCommand(unsigned int, unsigned int, unsigned int, DoCommandFlag, unsigned int, char const*) /home/jgr/openttd/trunk4/src/command.cpp:499
    #7 0x55d8d78c408c in CmdClearArea(unsigned int, DoCommandFlag, unsigned int, unsigned int, char const*) /home/jgr/openttd/trunk4/src/landscape.cpp:776
    #8 0x55d8d729fddf in DoCommandPInternal(unsigned int, unsigned int, unsigned int, unsigned int, void (*)(CommandCost const&, unsigned int, unsigned int, unsigned int, unsigned int), char const*, bool, bool) /home/jgr/openttd/trunk4/src/command.cpp:718
    #9 0x55d8d72a2864 in DoCommandP(unsigned int, unsigned int, unsigned int, unsigned int, void (*)(CommandCost const&, unsigned int, unsigned int, unsigned int, unsigned int), char const*, bool) /home/jgr/openttd/trunk4/src/command.cpp:582
    #10 0x55d8d8c46ea4 in GUIPlaceProcDragXY(ViewportDragDropSelectionProcess, unsigned int, unsigned int) /home/jgr/openttd/trunk4/src/terraform_gui.cpp:122
    #11 0x55d8d83595dc in BuildRailToolbarWindow::OnPlaceMouseUp(ViewportPlaceMethod, ViewportDragDropSelectionProcess, Point, unsigned int, unsigned int) /home/jgr/openttd/trunk4/src/rail_gui.cpp:711
    #12 0x55d8d8fdf14c in VpHandlePlaceSizingDrag() /home/jgr/openttd/trunk4/src/viewport.cpp:3299
    #13 0x55d8d9144924 in MouseLoop /home/jgr/openttd/trunk4/src/window.cpp:2866
    #14 0x55d8d9144924 in HandleMouseEvents() /home/jgr/openttd/trunk4/src/window.cpp:3031
    #15 0x55d8d8fa5f64 in VideoDriver_SDL::PollEvent() /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:540
    #16 0x55d8d8fa7502 in VideoDriver_SDL::MainLoop() /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:693
    #17 0x55d8d80340f9 in openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:860
    #18 0x7f59df519b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
    #19 0x55d8d6b73559 in _start (/home/jgr/openttd/trunk4/bin/openttd+0x3880559)

0x6250001ae1b8 is located 184 bytes inside of 9048-byte region [0x6250001ae100,0x6250001b0458)
freed by thread T0 here:
    #0 0x7f59e207e7b8 in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xde7b8)
    #1 0x55d8d89b3668 in Pool<BaseStation, unsigned short, 32ul, 64000ul, (PoolType)1, false, true>::FreeItem(unsigned long) /home/jgr/openttd/trunk4/src/core/pool_func.hpp:189
    #2 0x55d8d7811145 in Industry::~Industry() /home/jgr/openttd/trunk4/src/industry_cmd.cpp:158
    #3 0x55d8d7816ca7 in ClearTile_Industry /home/jgr/openttd/trunk4/src/industry_cmd.cpp:516
    #4 0x55d8d78c02f5 in CmdLandscapeClear(unsigned int, DoCommandFlag, unsigned int, unsigned int, char const*) /home/jgr/openttd/trunk4/src/landscape.cpp:724
    #5 0x55d8d729aeb6 in DoCommand(unsigned int, unsigned int, unsigned int, DoCommandFlag, unsigned int, char const*) /home/jgr/openttd/trunk4/src/command.cpp:499
    #6 0x55d8d78c408c in CmdClearArea(unsigned int, DoCommandFlag, unsigned int, unsigned int, char const*) /home/jgr/openttd/trunk4/src/landscape.cpp:776
    #7 0x55d8d729fddf in DoCommandPInternal(unsigned int, unsigned int, unsigned int, unsigned int, void (*)(CommandCost const&, unsigned int, unsigned int, unsigned int, unsigned int), char const*, bool, bool) /home/jgr/openttd/trunk4/src/command.cpp:718
    #8 0x55d8d72a2864 in DoCommandP(unsigned int, unsigned int, unsigned int, unsigned int, void (*)(CommandCost const&, unsigned int, unsigned int, unsigned int, unsigned int), char const*, bool) /home/jgr/openttd/trunk4/src/command.cpp:582
    #9 0x55d8d8c46ea4 in GUIPlaceProcDragXY(ViewportDragDropSelectionProcess, unsigned int, unsigned int) /home/jgr/openttd/trunk4/src/terraform_gui.cpp:122
    #10 0x55d8d83595dc in BuildRailToolbarWindow::OnPlaceMouseUp(ViewportPlaceMethod, ViewportDragDropSelectionProcess, Point, unsigned int, unsigned int) /home/jgr/openttd/trunk4/src/rail_gui.cpp:711
    #11 0x55d8d8fdf14c in VpHandlePlaceSizingDrag() /home/jgr/openttd/trunk4/src/viewport.cpp:3299
    #12 0x55d8d9144924 in MouseLoop /home/jgr/openttd/trunk4/src/window.cpp:2866
    #13 0x55d8d9144924 in HandleMouseEvents() /home/jgr/openttd/trunk4/src/window.cpp:3031
    #14 0x55d8d8fa5f64 in VideoDriver_SDL::PollEvent() /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:540
    #15 0x55d8d8fa7502 in VideoDriver_SDL::MainLoop() /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:693
    #16 0x55d8d80340f9 in openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:860
    #17 0x7f59df519b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)

previously allocated by thread T0 here:
    #0 0x7f59e207ed38 in __interceptor_calloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded38)
    #1 0x55d8d89b1d81 in Pool<BaseStation, unsigned short, 32ul, 64000ul, (PoolType)1, false, true>::GetNew(unsigned long) (/home/jgr/openttd/trunk4/bin/openttd+0x56bed81)
    #2 0x55d8d8a3b740 in Pool<BaseStation, unsigned short, 32ul, 64000ul, (PoolType)1, false, true>::PoolItem<&_station_pool>::operator new(unsigned long) /home/jgr/openttd/trunk4/src/core/pool_type.hpp:157
    #3 0x55d8d8a3b740 in BuildOilRig(unsigned int) /home/jgr/openttd/trunk4/src/station_cmd.cpp:4138
    #4 0x55d8d78c701b in RunTileLoop() /home/jgr/openttd/trunk4/src/landscape.cpp:833
    #5 0x55d8d80396e7 in StateGameLoop() /home/jgr/openttd/trunk4/src/openttd.cpp:1387
    #6 0x55d8d803fdeb in GameLoop() /home/jgr/openttd/trunk4/src/openttd.cpp:1475
    #7 0x55d8d8fa7d30 in VideoDriver_SDL::MainLoop() /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:735
    #8 0x55d8d80340f9 in openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:860
    #9 0x7f59df519b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)

SUMMARY: AddressSanitizer: heap-use-after-free /home/jgr/openttd/trunk4/src/tilearea.cpp:47 in OrthogonalTileArea::Add(unsigned int)
Shadow bytes around the buggy address:
  0x0c4a8002dbe0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a8002dbf0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a8002dc00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a8002dc10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a8002dc20: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c4a8002dc30: fd fd fd fd fd fd fd[fd]fd fd fd fd fd fd fd fd
  0x0c4a8002dc40: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c4a8002dc50: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c4a8002dc60: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c4a8002dc70: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c4a8002dc80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==26438==ABORTING

When the struct Station is deleted in DeleteOilRig, the Industry::neutral_station pointer is left dangling.
This pointer is dereferenced in CheckForDockingTile which is called within the struct Industry destructor.

Steps to reproduce

Remove an oil rig. Doing so with heap checking turned on makes the issue more obvious. Most of the time there is no crash.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@JGRennison