MPAS-Dev / MPAS-Model

Repository for MPAS models and shared framework releases.
231 stars 307 forks source link

Fix memory leaks due to double allocations on pointers #1160

Closed kuanchihwang closed 3 months ago

kuanchihwang commented 3 months ago

This PR fixes memory leaks due to duplicate allocate calls on pointers in the file src/core_atmosphere/inc/structs_and_variables.inc. This file is auto-generated and then included by src/core_atmosphere/mpas_atm_core_interface.F (among others) at compile time. No functional changes (NFC) are introduced by this PR.

For example, in the auto-generated file src/core_atmosphere/inc/structs_and_variables.inc, there are code snippets similar to the following:

subroutine atm_generate_pool_mesh(block, structPool, dimensionPool, packagePool)

! ... (Unrelated lines omitted) ...

      type(mpas_pool_type), pointer :: newSubPool

! ... (Unrelated lines omitted) ...

      allocate(newSubPool)
      call mpas_pool_create_pool(newSubPool)
      call mpas_pool_add_subpool(structPool, 'mesh', newSubPool)
      call mpas_pool_add_subpool(block % allStructs, 'mesh', newSubPool)

! ... (Unrelated lines omitted) ...

end subroutine atm_generate_pool_mesh

The problem is that the mpas_pool_create_pool subroutine already allocates the pointer in its arguments internally. Therefore, calling allocate before mpas_pool_create_pool actually causes memory leaks.

Consider this minimal reproducible example (MRE):

! $ cat mem_leak_demo.f90
program mem_leak_demo
    use mpas_derived_types, only: mpas_pool_type
    use mpas_pool_routines, only: mpas_pool_create_pool, mpas_pool_destroy_pool

    implicit none

    type(mpas_pool_type), pointer :: mpas_pool

    allocate(mpas_pool)
    call mpas_pool_create_pool(mpas_pool)
    call mpas_pool_destroy_pool(mpas_pool)
end program mem_leak_demo
! $ ifort -o mem_leak_demo mem_leak_demo.f90 -Isrc/external/esmf_time_f90 -Isrc/framework -Lsrc -lframework

Running the MRE with valgrind --leak-check=full ./mem_leak_demo produces the following report:

==32228== Memcheck, a memory error detector
==32228== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==32228== Using Valgrind-3.20.0 and LibVEX; rerun with -h for copyright info
==32228== Command: ./mem_leak_demo
==32228==
==32228==
==32228== HEAP SUMMARY:
==32228==     in use at exit: 144 bytes in 1 blocks
==32228==   total heap usage: 4 allocs, 3 frees, 1,432 bytes allocated
==32228==
==32228== 144 bytes in 1 blocks are definitely lost in loss record 1 of 1
==32228==    at 0x40366EB: malloc (vg_replace_malloc.c:393)
==32228==    by 0x4C7164: _mm_malloc (in <path-to>/MPAS-Model/mem_leak_demo)
==32228==    by 0x477B0C: for_allocate_handle (in <path-to>/MPAS-Model/mem_leak_demo)
==32228==    by 0x40A899: MAIN__ (in <path-to>/MPAS-Model/mem_leak_demo)
==32228==    by 0x40A83C: main (in <path-to>/MPAS-Model/mem_leak_demo)
==32228==
==32228== LEAK SUMMARY:
==32228==    definitely lost: 144 bytes in 1 blocks
==32228==    indirectly lost: 0 bytes in 0 blocks
==32228==      possibly lost: 0 bytes in 0 blocks
==32228==    still reachable: 0 bytes in 0 blocks
==32228==         suppressed: 0 bytes in 0 blocks
==32228==
==32228== For lists of detected and suppressed errors, rerun with: -s
==32228== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

Deleting the line with allocate(mpas_pool) in the MRE results in a clean report.

==112916== Memcheck, a memory error detector
==112916== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==112916== Using Valgrind-3.20.0 and LibVEX; rerun with -h for copyright info
==112916== Command: ./mem_leak_demo
==112916==
==112916==
==112916== HEAP SUMMARY:
==112916==     in use at exit: 0 bytes in 0 blocks
==112916==   total heap usage: 3 allocs, 3 frees, 1,288 bytes allocated
==112916==
==112916== All heap blocks were freed -- no leaks are possible
==112916==
==112916== For lists of detected and suppressed errors, rerun with: -s
==112916== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
mgduda commented 3 months ago

@kuanchihwang Excellent work in finding this memory leak! We're planning to release v8.1.0 later this week, and since this is a bug fix, I think it makes sense to set the base branch to release-v8.1.0. Would you mind rebasing your PR branch on, say, the v8.0.0 tag? If you'd rather, I can handle the rebase and force-push the branch to your fork; just let me know what you'd prefer.

kuanchihwang commented 3 months ago

Yes, I have rebased this PR onto the release-v8.1.0 branch. The history is now linear again.