suever / pydicom-experimental

pydicom test
0 stars 1 forks source link

Memory error is produced when reading a non-DICOM file #75

Closed suever closed 9 years ago

suever commented 9 years ago

From almar.klein@gmail.com on February 16, 2010 06:14:19

What steps will reproduce the problem? Try to read a file that is not a dicom file. What is the expected output? What do you see instead? I would expect an Exception being thrown telling me the file is not dicom. The read-function could easily check this by looking for "DICM" at position 128 in the file. Instead, a Memory error is produced. What version of the product are you using? 0.9.4

Original issue: http://code.google.com/p/pydicom/issues/detail?id=75

suever commented 9 years ago

From almar.klein@gmail.com on February 16, 2010 03:22:56

After examining the source code, I see that a missing header is assumed when the "DICM" string is not found. I don't know whether this occurs often, but strictly speaking, such a file is not valid dicom, and IMO should not be processed.

In my application, I use CT data, which is stored in several hundred dicom files in a directory. Sometimes there may be a text file or xml file with a summary of the data of some sort. I could of course test for the extension, but catching a NotValidDicom exception (or something like that) would be more beautiful (and would make my application work whatever extra data I'd like to put with the dicom files).

I'd be happy to submit a patch if you agree with me.

suever commented 9 years ago

From darcymason@gmail.com on February 17, 2010 19:45:22

almar.klein wrote: ... catching a NotValidDicom exception (or something like that) would be more beautiful (and would make my application work whatever extra data I'd like to put with the dicom files). I'd be happy to submit a patch if you agree with me.

This has been discussed in issue 70 , so I'm flagging this as a duplicate. But yes, I'd be happy to accept a patch (against the new 0.9.4-1 if possible). But it should include using an optional argument "force=False" as discussed in issue 70 , which (if set True) would retain the current behaviour, else raise an exception as you suggested.

Status: Duplicate
Labels: Milestone-NextRelease

suever commented 9 years ago

From almar.klein@gmail.com on February 18, 2010 00:33:11

Ah, I'm sorry for not properly searching the issue list before submitting... Will look into creating a patch.

suever commented 9 years ago

From darcymason@gmail.com on February 26, 2010 07:28:06

This issue was closed by revision a7c9dce062 .

Status: Fixed