Skip to content

[Codescan] Make the CIF expression parser safe to initialize more than once #33

Description

@Stardust0831

Global scan provenance

This issue was found by the Codex global repository scan of every non-unit-test file in Stardust0831/Multiwfn, pinned to commit d2b770a531eb7039153198d04520b5992f15c49d.

Problem

The function parser stores Comp as a saved module pointer. initf() allocates it unconditionally and exposes no finalizer that deallocates the existing component buffers and the outer pointer before another initialization.

Multiwfn/fparser.f90

Lines 92 to 118 in d2b770a

TYPE tComp
INTEGER(is), DIMENSION(:), POINTER :: ByteCode
INTEGER :: ByteCodeSize
REAL(rn), DIMENSION(:), POINTER :: Immed
INTEGER :: ImmedSize
REAL(rn), DIMENSION(:), POINTER :: Stack
INTEGER :: StackSize, &
StackPtr
END TYPE tComp
TYPE (tComp), DIMENSION(:), POINTER :: Comp ! Bytecode
INTEGER, DIMENSION(:), ALLOCATABLE :: ipos ! Associates function strings
!
CONTAINS
!
SUBROUTINE initf (n)
!----- -------- --------- --------- --------- --------- --------- --------- -------
! Initialize function parser for n functions
!----- -------- --------- --------- --------- --------- --------- --------- -------
IMPLICIT NONE
INTEGER, INTENT(in) :: n ! Number of functions
INTEGER :: i
!----- -------- --------- --------- --------- --------- --------- --------- -------
ALLOCATE (Comp(n))
DO i=1,n
NULLIFY (Comp(i)%ByteCode,Comp(i)%Immed,Comp(i)%Stack)
END DO
END SUBROUTINE initf

The CIF reader calls initf(3) whenever symmetry operations are processed:

Multiwfn/fileIO.f90

Lines 9430 to 9473 in d2b770a

if (infomode==0) write(*,*) "Loading symmetry opteration and replicate atoms"
call loclabel(10,"_symmetry_equiv_pos_as_xyz",ifound)
if (ifound==0) call loclabel(10,"_space_group_symop_operation_xyz",ifound)
if (ifound==1) then
do while(.true.) !Load to loop_ prior to symmetry operation field
backspace(10)
read(10,"(a)") c80tmp
if (index(c80tmp,"loop_")/=0) exit
backspace(10)
end do
nlab=0
do while(.true.)
read(10,*) c80tmp
if (index(c80tmp,'_')==0) exit
nlab=nlab+1
if (index(c80tmp,'_symmetry_equiv_pos_as_xyz')/=0.or.index(c80tmp,'_space_group_symop_operation_xyz')/=0) isymop=nlab
end do
backspace(10)
!Read symmetry operation string
nsymopstr=0 !Number of symmetry operations
do while(.true.)
read(10,"(a)",iostat=ierror) c80tmp
if (c80tmp==" ".or.index(c80tmp,'#')/=0.or.index(c80tmp,'_')/=0.or.ierror/=0) exit
nsymopstr=nsymopstr+1
do ichar=1,len_trim(c80tmp) !Replace all ' with space, make coordinate variables to lower case
if (c80tmp(ichar:ichar)=="'") c80tmp(ichar:ichar)=" "
if (c80tmp(ichar:ichar)=="X") c80tmp(ichar:ichar)="x"
if (c80tmp(ichar:ichar)=="Y") c80tmp(ichar:ichar)="y"
if (c80tmp(ichar:ichar)=="Z") c80tmp(ichar:ichar)="z"
end do
c80tmp=adjustl(c80tmp)
if (nlab==1) then
symopstr(nsymopstr)=trim(c80tmp)
else !Load the second term, assume at most there are two terms in a line
itmp=index(c80tmp,' ')
symopstr(nsymopstr)=trim(adjustl(c80tmp(itmp+1:)))
end if
end do
if (infomode==0) write(*,"(a,i4)") " Number of symmetry operations:",nsymopstr
allocate(a_dup(ncenter_tmp*nsymopstr))
call initf(3)
ncenter_dup=ncenter_tmp
a_dup(1:ncenter_dup)=a_tmp

Multiwfn explicitly supports loading another file in the same process through the r menu command:

Multiwfn/Multiwfn.f90

Lines 309 to 316 in d2b770a

if (c200tmp=="q".or.c200tmp=="-10") then !Exit program
stop
else if (c200tmp=="r".or.c200tmp=="-11") then !Load a new file
call dealloall(0)
call dealloall_org
filename=""
ifirstMultiwfn=0
goto 10

The normal global cleanup does not own or release the private parser state. Therefore, loading a CIF with symmetry operations and then reloading another such CIF reaches allocate(Comp(n)) while Comp is still associated. A conforming Fortran runtime reports an allocation error instead of loading the second file.

Impact

The documented same-process file reload workflow can terminate when CIF files are loaded more than once. Even if a compiler/runtime extension tolerates the outer reallocation, the bytecode, immediate-value and stack buffers referenced by the previous components would be leaked.

Suggested direction

  • Add a public parser finalization/reset routine that deallocates every associated ByteCode, Immed and Stack pointer, then deallocates/nullifies Comp and cleans any parser work arrays.
  • Make initf() idempotent by invoking that cleanup when Comp is already associated.
  • Call the parser cleanup from the normal whole-system cleanup path as well as before reinitialization.
  • Add a functional regression that loads two CIF files with symmetry operations through the same process and verifies the second structure is parsed successfully.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions