Starlink / starlink

Starlink Software Collection
163 stars 58 forks source link

Memory corruption in fits2ndf #6

Closed timj closed 11 years ago

timj commented 11 years ago

Jane Buckle reports that she has memory corruption in a FITS file:

I've created a fits file (written using pyfits, copying a header), which gaia is happy to display, but kappa commands (such as ndftrace) complain about:

The error starts with:

*** glibc detected *** /stardev/bin/convert/fits2ndf: double free or corruption (!prev): 0x0000000004505e50 ***

Then there is a long listing which it call a memory map, then it complains about the fits2ndf:

2b80815dd000-2b80815de000 rw-p 00000000 00:1
!! Requested data extends beyond the end of the record; record length is 0
!     bytes (possible corrupt HDS container file

!  HDS_OPEN: Error opening an HDS container file.
!  Failed to convert the FITS format file

!  NDFTRACE: Error displaying the attributes of an NDF data structure.
!  Application exit status NDF__CVTER, foreign format conversion error

I don't believe that the file is actually corrupt, since gaia has no problems.

timj commented 11 years ago

I have a test file from Jane and it looks like there is an invalid write in cof_chisr.f:

==70292== Invalid write of size 4
==70292==    at 0x100021B2C: cof_chisr_ (cof_chisr.f:500)
==70292==    by 0x10000000F: ??? (in /star/bin/convert/fits2ndf)
==70292==    by 0x7B7B7A7A0000004F: ???
==70292==    by 0x1B007E7E00000013: ???
==70292==    by 0x10491B82F: ???
==70292==    by 0x100000000: ??? (in /star/bin/convert/fits2ndf)
==70292==  Address 0x1001bd3f8 is 0 bytes after a block of size 1,704 alloc'd
==70292==    at 0x58D3: calloc (in /usr/local/Cellar/valgrind/3.8.1/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==70292==    by 0x3FA3F49: Malloc (cnfMem.c:381)
==70292==    by 0x3F807EC: psx_calloc_ (psx_calloc.c:245)
==70292==    by 0x1000532EE: cof_smfim_ (cof_smfim.f:496)
timj commented 11 years ago

The test file I have does seem to have originally come from an NDF so I wonder if the HISTORY has been tweaked in such a manner that cof_chisr.f is confused.

MalcolmCurrie commented 11 years ago

The FITSIO routine FTGHSP purports to return the number of headers excluding the END card. However, it returns the number excluding trailing blank cards. The documentation for this routine does not state this. This is an efficiency feature according to Bill Pence. Fair enough. My quibble is with the incomplete documentation.

FITS2NDF allocates workspace to a LOGICAL array to the number of headers to record which ones are to appear in the airlock. Later it encounters an extra blank header not counted by FTGHSP and so exceeds that array's bounds. FITS2NDF crashed when freeing the workspace.

I have modified COF_NHEAD to account for such blank lines and thus return the full count.

timj commented 11 years ago

The fix was in commit b950d6175926b12f4c426c41eeb8db6631d342ff