ofi BTL reading uninitialized values during finalize

[jsquyres]

While doing some other testing, I noticed that the OFI BTL is doing some bad reads during MPI_Finalize. I don't know if these are actually causing problems anywhere, but they certainly have the potential to do so.

Can someone from Intel please have a look?

$ valgrind --num-callers=50 --tool=memcheck ./hello_c 
==2379== Memcheck, a memory error detector
==2379== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==2379== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==2379== Command: ./hello_c
==2379== 
==2379== Conditional jump or move depends on uninitialised value(s)
==2379==    at 0x535B307: opal_interval_tree_reader_get_token (opal_interval_tree.c:126)
==2379==    by 0x535C8A2: opal_interval_tree_traverse (opal_interval_tree.c:705)
==2379==    by 0x54287C2: mca_rcache_base_vma_tree_iterate (rcache_base_vma_tree.c:105)
==2379==    by 0x5428391: mca_rcache_base_vma_iterate (rcache_base_vma.c:153)
==2379==    by 0xA103374: mca_rcache_grdma_finalize (rcache_grdma_module.c:543)
==2379==    by 0x5427D16: mca_rcache_base_module_destroy (rcache_base_create.c:113)
==2379==    by 0xA7179A4: mca_btl_ofi_finalize (btl_ofi_module.c:317)
==2379==    by 0x53C3172: mca_btl_base_close (btl_base_frame.c:203)
==2379==    by 0x53A5F6F: mca_base_framework_close (mca_base_framework.c:252)
==2379==    by 0x4CFCFD9: mca_bml_base_close (bml_base_frame.c:130)
==2379==    by 0x53A5F6F: mca_base_framework_close (mca_base_framework.c:252)
==2379==    by 0x4C76C01: ompi_mpi_finalize (ompi_mpi_finalize.c:449)
==2379==    by 0x4CB62F1: PMPI_Finalize (pfinalize.c:54)
==2379==    by 0x4008E9: main (hello_c.c:24)
==2379== 
==2379== Use of uninitialised value of size 8
==2379==    at 0x535A332: opal_thread_compare_exchange_strong_32 (thread_usage.h:156)
==2379==    by 0x535B34E: opal_interval_tree_reader_get_token (opal_interval_tree.c:133)
==2379==    by 0x535C8A2: opal_interval_tree_traverse (opal_interval_tree.c:705)
==2379==    by 0x54287C2: mca_rcache_base_vma_tree_iterate (rcache_base_vma_tree.c:105)
==2379==    by 0x5428391: mca_rcache_base_vma_iterate (rcache_base_vma.c:153)
==2379==    by 0xA103374: mca_rcache_grdma_finalize (rcache_grdma_module.c:543)
==2379==    by 0x5427D16: mca_rcache_base_module_destroy (rcache_base_create.c:113)
==2379==    by 0xA7179A4: mca_btl_ofi_finalize (btl_ofi_module.c:317)
==2379==    by 0x53C3172: mca_btl_base_close (btl_base_frame.c:203)
==2379==    by 0x53A5F6F: mca_base_framework_close (mca_base_framework.c:252)
==2379==    by 0x4CFCFD9: mca_bml_base_close (bml_base_frame.c:130)
==2379==    by 0x53A5F6F: mca_base_framework_close (mca_base_framework.c:252)
==2379==    by 0x4C76C01: ompi_mpi_finalize (ompi_mpi_finalize.c:449)
==2379==    by 0x4CB62F1: PMPI_Finalize (pfinalize.c:54)
==2379==    by 0x4008E9: main (hello_c.c:24)
==2379== 
==2379== Use of uninitialised value of size 8
==2379==    at 0x535A34B: opal_thread_compare_exchange_strong_32 (thread_usage.h:156)
==2379==    by 0x535B34E: opal_interval_tree_reader_get_token (opal_interval_tree.c:133)
==2379==    by 0x535C8A2: opal_interval_tree_traverse (opal_interval_tree.c:705)
==2379==    by 0x54287C2: mca_rcache_base_vma_tree_iterate (rcache_base_vma_tree.c:105)
==2379==    by 0x5428391: mca_rcache_base_vma_iterate (rcache_base_vma.c:153)
==2379==    by 0xA103374: mca_rcache_grdma_finalize (rcache_grdma_module.c:543)
==2379==    by 0x5427D16: mca_rcache_base_module_destroy (rcache_base_create.c:113)
==2379==    by 0xA7179A4: mca_btl_ofi_finalize (btl_ofi_module.c:317)
==2379==    by 0x53C3172: mca_btl_base_close (btl_base_frame.c:203)
==2379==    by 0x53A5F6F: mca_base_framework_close (mca_base_framework.c:252)
==2379==    by 0x4CFCFD9: mca_bml_base_close (bml_base_frame.c:130)
==2379==    by 0x53A5F6F: mca_base_framework_close (mca_base_framework.c:252)
==2379==    by 0x4C76C01: ompi_mpi_finalize (ompi_mpi_finalize.c:449)
==2379==    by 0x4CB62F1: PMPI_Finalize (pfinalize.c:54)
==2379==    by 0x4008E9: main (hello_c.c:24)
==2379== 
==2379== Use of uninitialised value of size 8
==2379==    at 0x535B390: opal_interval_tree_reader_return_token (opal_interval_tree.c:141)
==2379==    by 0x535C8F1: opal_interval_tree_traverse (opal_interval_tree.c:707)
==2379==    by 0x54287C2: mca_rcache_base_vma_tree_iterate (rcache_base_vma_tree.c:105)
==2379==    by 0x5428391: mca_rcache_base_vma_iterate (rcache_base_vma.c:153)
==2379==    by 0xA103374: mca_rcache_grdma_finalize (rcache_grdma_module.c:543)
==2379==    by 0x5427D16: mca_rcache_base_module_destroy (rcache_base_create.c:113)
==2379==    by 0xA7179A4: mca_btl_ofi_finalize (btl_ofi_module.c:317)
==2379==    by 0x53C3172: mca_btl_base_close (btl_base_frame.c:203)
==2379==    by 0x53A5F6F: mca_base_framework_close (mca_base_framework.c:252)
==2379==    by 0x4CFCFD9: mca_bml_base_close (bml_base_frame.c:130)
==2379==    by 0x53A5F6F: mca_base_framework_close (mca_base_framework.c:252)
==2379==    by 0x4C76C01: ompi_mpi_finalize (ompi_mpi_finalize.c:449)
==2379==    by 0x4CB62F1: PMPI_Finalize (pfinalize.c:54)
==2379==    by 0x4008E9: main (hello_c.c:24)

[aravindksg]

I'm able to reproduce this, I'll look into it

[aravindksg]

It doesn't seem like the OFI BTL was using an uninitialized pointer. Simply setting the rcache pointer value to NULL after the destroy call seems to work:

     if (ofi_btl->rcache) {
         mca_rcache_base_module_destroy (ofi_btl->rcache);
+        ofi_btl->rcache = NULL;
     }

valgrind --num-callers=50 --tool=memcheck --track-origins=yes ./mpi_hello
==142256== Memcheck, a memory error detector
==142256== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==142256== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==142256== Command: ./mpi_hello
==142256==
Hello from proc rank 0 out of rank 1
==142256==
==142256== HEAP SUMMARY:
==142256==     in use at exit: 20,161 bytes in 59 blocks
==142256==   total heap usage: 17,230 allocs, 17,171 frees, 4,845,995 bytes allocated
==142256==
==142256== LEAK SUMMARY:
==142256==    definitely lost: 49 bytes in 3 blocks
==142256==    indirectly lost: 95 bytes in 3 blocks
==142256==      possibly lost: 0 bytes in 0 blocks
==142256==    still reachable: 20,017 bytes in 53 blocks
==142256==         suppressed: 0 bytes in 0 blocks
==142256== Rerun with --leak-check=full to see details of leaked memory
==142256==
==142256== For counts of detected and suppressed errors, rerun with: -v
==142256== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

[jsquyres]

Sweet! Amazing it didn't segv before this...

[bertwesarg]

Would it make sense to audit the code and set all pointers after a call to mca_rcache_base_module_destroy() to NULL? Only 3 out of 7 callsites in the master/v3.1.x/v4.0.x branches reset the pointer after the call.