From f9b9d3595a8331ee006e281168ca34e60a31b7a7 Mon Sep 17 00:00:00 2001 From: Andrew Benson Date: Thu, 14 Dec 2023 09:14:10 -0800 Subject: [PATCH 1/3] fix: Fix a segfault associated with output of `rank?VarLen` datasets When the ouptut buffers must be extended, the `rank1VarLen` and `rank2VarLen` arrays must be extended and their original content copied back in. The was previously not done, resulting in a segfault due to out-of-bounds array access. --- source/merger_trees.outputter.standard.F90 | 58 ++- .../regressions/outputRank2ExtendSegFault.xml | 464 ++++++++++++++++++ 2 files changed, 509 insertions(+), 13 deletions(-) create mode 100644 testSuite/regressions/outputRank2ExtendSegFault.xml diff --git a/source/merger_trees.outputter.standard.F90 b/source/merger_trees.outputter.standard.F90 index 4b4584c8f5..9836d767b6 100644 --- a/source/merger_trees.outputter.standard.F90 +++ b/source/merger_trees.outputter.standard.F90 @@ -477,7 +477,7 @@ subroutine standardOutput(self,node,time) doubleArray =extractor_%extract (node,time,instance) do i=1,+extractor_%elementCount( time) if ( allocated(self%doubleProperty (doubleProperty +i)%scalar)) deallocate(self%doubleProperty (doubleProperty +i)%scalar) - if ( allocated(self%doubleProperty (doubleProperty +i)%rank1 )) then + if ( allocated(self%doubleProperty (doubleProperty +i)%rank1 )) then if (size(self%doubleProperty (doubleProperty +i)%rank1,dim=1) /= size(doubleArray,dim=1)) deallocate(self%doubleProperty (doubleProperty +i)%rank1 ) end if if (.not.allocated(self%doubleProperty (doubleProperty +i)%rank1)) allocate(self%doubleProperty (doubleProperty +i)%rank1(size(doubleArray,dim=1),self%doubleBufferSize)) @@ -513,7 +513,11 @@ subroutine standardOutput(self,node,time) if ( allocated(self%doubleProperty (doubleProperty +i)%rank1VarLen)) deallocate(self%doubleProperty (doubleProperty +i)%rank1VarLen) if (.not.allocated(self%doubleProperty (doubleProperty +i)%rank2VarLen)) allocate(self%doubleProperty (doubleProperty +i)%rank2VarLen(self%doubleBufferSize)) if (associated(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row)) then - if (size(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row) /= size(doubleArray2D,dim=1)) deallocate(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row) + if ( & + & size(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row,dim=1) /= size(doubleArray2D,dim=1) & + & .or. & + & size(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row,dim=2) /= size(doubleArray2D,dim=2) & + & ) deallocate(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row) end if if (.not.associated(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row)) then allocate(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row(size(doubleArray2D,dim=1),size(doubleArray2D,dim=2))) @@ -573,7 +577,11 @@ subroutine standardOutput(self,node,time) if (.not.allocated(self%doubleProperty(doubleProperty +i)%rank2VarLen)) allocate(self%doubleProperty (doubleProperty +i)%rank2VarLen(self%doubleBufferSize)) if (associated(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row)) then shape_=doubleProperties(i)%shape() - if (size(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row) /= shape_(1)) deallocate(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row) + if ( & + & size(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row,dim=1) /= shape_(1) & + & .or. & + & size(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row,dim=2) /= shape_(2) & + & ) deallocate(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row) deallocate(shape_) end if if (.not.associated(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row)) then @@ -828,28 +836,52 @@ subroutine standardExtendDoubleBuffer(self) !!{ Extend the size of the double buffer. !!} + use :: IO_HDF5, only : hdf5VarDouble , hdf5VarDouble2D implicit none class (mergerTreeOutputterStandard), intent(inout) :: self double precision , allocatable , dimension(: ) :: scalarTemporary double precision , allocatable , dimension(:,:) :: rank1Temporary - integer :: i + type (hdf5VarDouble ), allocatable , dimension(: ) :: rank1VarLenTemporary + type (hdf5VarDouble2D ), allocatable , dimension(: ) :: rank2VarLenTemporary + integer :: i , j do i=1,size(self%doubleProperty) - if (allocated(self%doubleProperty(i)%scalar)) then - call move_alloc(self%doubleProperty(i)%scalar,scalarTemporary) - allocate(self%doubleProperty(i)%scalar( self%doubleBufferSize+standardBufferSizeIncrement)) - self%doubleProperty(i)%scalar( 1:self%doubleBufferSize)=scalarTemporary + if (allocated(self%doubleProperty(i)%scalar )) then + call move_alloc(self%doubleProperty(i)%scalar ,scalarTemporary) + allocate(self%doubleProperty(i)%scalar ( self%doubleBufferSize+standardBufferSizeIncrement)) + self%doubleProperty(i)%scalar ( 1:self%doubleBufferSize)=scalarTemporary deallocate(scalarTemporary) end if - if (allocated(self%doubleProperty(i)%rank1 )) then - call move_alloc(self%doubleProperty(i)%rank1 ,rank1Temporary ) - allocate(self%doubleProperty(i)%rank1 (size(rank1Temporary,dim=1),self%doubleBufferSize+standardBufferSizeIncrement)) - self%doubleProperty(i)%rank1 (:,1:self%doubleBufferSize)=rank1Temporary + if (allocated(self%doubleProperty(i)%rank1 )) then + call move_alloc(self%doubleProperty(i)%rank1 ,rank1Temporary ) + allocate(self%doubleProperty(i)%rank1 (size(rank1Temporary,dim=1),self%doubleBufferSize+standardBufferSizeIncrement)) + self%doubleProperty(i)%rank1 (:,1:self%doubleBufferSize)=rank1Temporary deallocate(rank1Temporary ) end if + if (allocated(self%doubleProperty(i)%rank1VarLen)) then + call move_alloc(self%doubleProperty(i)%rank1VarLen,rank1VarLenTemporary) + allocate(self%doubleProperty(i)%rank1VarLen( self%doubleBufferSize+standardBufferSizeIncrement)) + self%doubleProperty(i)%rank1VarLen( 1:self%doubleBufferSize)=rank1VarLenTemporary + ! Nullify row pointers in the temporary array to avoid the associated data being destroyed when our temporary array is + ! deallocated. + do j=1,size(rank1VarLenTemporary) + rank1VarLenTemporary(j)%row => null() + end do + deallocate(rank1VarLenTemporary) + end if + if (allocated(self%doubleProperty(i)%rank2VarLen)) then + call move_alloc(self%doubleProperty(i)%rank2VarLen,rank2VarLenTemporary) + allocate(self%doubleProperty(i)%rank2VarLen( self%doubleBufferSize+standardBufferSizeIncrement)) + self%doubleProperty(i)%rank2VarLen( 1:self%doubleBufferSize)=rank2VarLenTemporary + ! Nullify row pointers in the temporary array to avoid the associated data being destroyed when our temporary array is + ! deallocated. + do j=1,size(rank2VarLenTemporary) + rank2VarLenTemporary(j)%row => null() + end do + deallocate(rank2VarLenTemporary) + end if end do self%doubleBufferSize=self%doubleBufferSize+standardBufferSizeIncrement - return end subroutine standardExtendDoubleBuffer diff --git a/testSuite/regressions/outputRank2ExtendSegFault.xml b/testSuite/regressions/outputRank2ExtendSegFault.xml new file mode 100644 index 0000000000..3218e928ce --- /dev/null +++ b/testSuite/regressions/outputRank2ExtendSegFault.xml @@ -0,0 +1,464 @@ + + + + + + 2 + 0.9.4 + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + From 174953b7773fb8fc386660daaa862a734910319f Mon Sep 17 00:00:00 2001 From: Andrew Benson Date: Thu, 14 Dec 2023 11:34:16 -0800 Subject: [PATCH 2/3] feat: Add a new regression test for the `rank2VarLen` output segfault --- .github/workflows/cicd.yml | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/.github/workflows/cicd.yml b/.github/workflows/cicd.yml index ff4cdebf52..9281350cd2 100644 --- a/.github/workflows/cicd.yml +++ b/.github/workflows/cicd.yml @@ -8815,6 +8815,42 @@ jobs: ./Galacticus.exe testSuite/regressions/treeWithNoPrimaryProgenitor.xml 2>&1 | tee test.log ! grep -q FAIL test.log - run: echo "This job's status is ${{ job.status }}." + Regressions-Output-Rank2VarLen-SegFault: + runs-on: ubuntu-latest + container: ghcr.io/galacticusorg/buildenv:latest + needs: Build-Executable-Linux + steps: + - run: echo "The job was automatically triggered by a ${{ github.event_name }} event." + - run: echo "This job is now running on a ${{ runner.os }} server." + - run: echo "The name of the branch is ${{ github.ref }} and the repository is ${{ github.repository }}." + - name: Check out repository code + uses: actions/checkout@v4 + - name: Check out repository datasets + uses: actions/checkout@v4 + with: + repository: galacticusorg/datasets + path: datasets + - run: echo "The ${{ github.repository }} repository has been cloned to the runner." + - name: "Set environmental variables" + run: | + echo "GALACTICUS_EXEC_PATH=$GITHUB_WORKSPACE" >> $GITHUB_ENV + echo "GALACTICUS_DATA_PATH=$GITHUB_WORKSPACE/datasets" >> $GITHUB_ENV + - name: Download executables + uses: actions/download-artifact@v3 + with: + name: galacticus-exec + - name: Create test suite output directory + run: mkdir -p $GALACTICUS_EXEC_PATH/testSuite/outputs + - name: Run test + run: | + cd $GALACTICUS_EXEC_PATH + git config --global --add safe.directory $GALACTICUS_EXEC_PATH + chmod u=wrx ./Galacticus.exe + mkdir -p testSuite/outputs/regressions + set -o pipefail + ./Galacticus.exe testSuite/regressions/outputRank2ExtendSegFault.xml 2>&1 | tee test.log + ! grep -q FAIL test.log + - run: echo "This job's status is ${{ job.status }}." Regressions-Bar-Instability-FPE: runs-on: ubuntu-latest container: ghcr.io/galacticusorg/buildenv:latest From 73004ce7609dc32767fbfe65b2bb3775d591c5cd Mon Sep 17 00:00:00 2001 From: Andrew Benson Date: Thu, 14 Dec 2023 12:59:52 -0800 Subject: [PATCH 3/3] fix: Add new regression test to the dependencies for deployment --- .github/workflows/cicd.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/cicd.yml b/.github/workflows/cicd.yml index 9281350cd2..7020404e67 100644 --- a/.github/workflows/cicd.yml +++ b/.github/workflows/cicd.yml @@ -10561,6 +10561,7 @@ jobs: Regressions-Tree-With-Initial-Satellite-In-Progenitorless-Host, Regressions-Tree-With-No-Primary-Progenitor, Regressions-Bar-Instability-FPE, + Regressions-Output-Rank2VarLen-SegFault, Test-Methods-01, Test-Methods-02, Test-Methods-03,