Repository: pydicom/pydicom

Issue:
Malformed PixelData
I am trying to convert color spaces using:

``` python
        arr = convert_ybr_to_rgb(ds.pixel_array)
        ds.file_meta.TransferSyntaxUID = pydicom.uid.ExplicitVRLittleEndian
        ds.is_little_endian = True
        ds.is_implicit_VR = False
        ds.PixelData = arr.tostring()
        ds.PlanarConfiguration = 0
        ds.PhotometricInterpretation = 'RGB'
```

However `dcmcjpeg` then barfs when it tries to open the saved file with:

```
W: DcmSequenceOfItems: Length of item in sequence PixelData (7fe0,0010) is odd
E: DcmSequenceOfItems: Parse error in sequence (7fe0,0010), found (292a,2a48) instead of sequence delimiter (fffe,e0dd)
F: Sequence Delimitation Item missing: reading file: /var/folders/nk/5v0p39pn4yg7c_3vtydljk000000gn/T/tmpLZfsSL.dcm
```


Discussion:
pep8speaks: Hello @mrbean-bremen! Thanks for updating the PR.

 - In the file [`pydicom/filewriter.py`](https://github.com/pydicom/pydicom/blob/dd7516dd80edd1270b7b8fac567b5dfc9aa4e1e1/pydicom/filewriter.py), following are the PEP8 issues :

> [Line 417:80](https://github.com/pydicom/pydicom/blob/dd7516dd80edd1270b7b8fac567b5dfc9aa4e1e1/pydicom/filewriter.py#L417): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (81 > 79 characters)
> [Line 421:80](https://github.com/pydicom/pydicom/blob/dd7516dd80edd1270b7b8fac567b5dfc9aa4e1e1/pydicom/filewriter.py#L421): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (88 > 79 characters)
> [Line 423:80](https://github.com/pydicom/pydicom/blob/dd7516dd80edd1270b7b8fac567b5dfc9aa4e1e1/pydicom/filewriter.py#L423): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (88 > 79 characters)




darcymason: @glemaitre, I'm assuming no further comments and going ahead to merge; please add a comment if needed.

Patch:
diff --git a/pydicom/filewriter.py b/pydicom/filewriter.py
index 4181cce16..efb596845 100644
--- a/pydicom/filewriter.py
+++ b/pydicom/filewriter.py
@@ -415,6 +415,16 @@ def write_data_element(fp, data_element, encoding=default_encoding):
     if (hasattr(data_element, "is_undefined_length")
             and data_element.is_undefined_length):
         is_undefined_length = True
+        # valid pixel data with undefined length shall contain encapsulated
+        # data, e.g. sequence items - raise ValueError otherwise (see #238)
+        if data_element.tag == 0x7fe00010:  # pixel data
+            val = data_element.value
+            if (fp.is_little_endian and not
+                    val.startswith(b'\xfe\xff\x00\xe0') or
+                    not fp.is_little_endian and
+                    not val.startswith(b'\xff\xfe\xe0\x00')):
+                raise ValueError('Pixel Data with undefined length must '
+                                 'start with an item tag')
     location = fp.tell()
     fp.seek(length_location)
     if not fp.is_implicit_VR and VR not in extra_length_VRs:


Test patch:
diff --git a/pydicom/tests/test_filewriter.py b/pydicom/tests/test_filewriter.py
index 70567f134..4d7814ed0 100644
--- a/pydicom/tests/test_filewriter.py
+++ b/pydicom/tests/test_filewriter.py
@@ -41,7 +41,6 @@ except AttributeError:
     except ImportError:
         print("unittest2 is required for testing in python2.6")
 
-
 rtplan_name = get_testdata_files("rtplan.dcm")[0]
 rtdose_name = get_testdata_files("rtdose.dcm")[0]
 ct_name = get_testdata_files("CT_small.dcm")[0]
@@ -263,8 +262,8 @@ class WriteDataElementTests(unittest.TestCase):
         # Was issue 74
         data_elem = DataElement(0x00280009, "AT", [])
         expected = hex2bytes((
-            " 28 00 09 00"   # (0028,0009) Frame Increment Pointer
-            " 00 00 00 00"   # length 0
+            " 28 00 09 00"  # (0028,0009) Frame Increment Pointer
+            " 00 00 00 00"  # length 0
         ))
         write_data_element(self.f1, data_elem)
         got = self.f1.getvalue()
@@ -1731,6 +1730,74 @@ class TestWriteFileMetaInfoNonStandard(unittest.TestCase):
         self.assertEqual(meta, ref_meta)
 
 
+class TestWriteUndefinedLengthPixelData(unittest.TestCase):
+    """Test write_data_element() for pixel data with undefined length."""
+
+    def setUp(self):
+        self.fp = DicomBytesIO()
+
+    def test_little_endian_correct_data(self):
+        """Pixel data starting with an item tag is written."""
+        self.fp.is_little_endian = True
+        self.fp.is_implicit_VR = False
+        pixel_data = DataElement(0x7fe00010, 'OB',
+                                 b'\xfe\xff\x00\xe0'
+                                 b'\x00\x01\x02\x03',
+                                 is_undefined_length=True)
+        write_data_element(self.fp, pixel_data)
+
+        expected = (b'\xe0\x7f\x10\x00'  # tag
+                    b'OB\x00\x00'  # VR
+                    b'\xff\xff\xff\xff'  # length
+                    b'\xfe\xff\x00\xe0\x00\x01\x02\x03'  # contents
+                    b'\xfe\xff\xdd\xe0\x00\x00\x00\x00')  # SQ delimiter
+        self.fp.seek(0)
+        assert self.fp.read() == expected
+
+    def test_big_endian_correct_data(self):
+        """Pixel data starting with an item tag is written."""
+        self.fp.is_little_endian = False
+        self.fp.is_implicit_VR = False
+        pixel_data = DataElement(0x7fe00010, 'OB',
+                                 b'\xff\xfe\xe0\x00'
+                                 b'\x00\x01\x02\x03',
+                                 is_undefined_length=True)
+        write_data_element(self.fp, pixel_data)
+        expected = (b'\x7f\xe0\x00\x10'  # tag
+                    b'OB\x00\x00'  # VR
+                    b'\xff\xff\xff\xff'  # length
+                    b'\xff\xfe\xe0\x00\x00\x01\x02\x03'  # contents
+                    b'\xff\xfe\xe0\xdd\x00\x00\x00\x00')  # SQ delimiter
+        self.fp.seek(0)
+        assert self.fp.read() == expected
+
+    def test_little_endian_incorrect_data(self):
+        """Writing pixel data not starting with an item tag raises."""
+        self.fp.is_little_endian = True
+        self.fp.is_implicit_VR = False
+        pixel_data = DataElement(0x7fe00010, 'OB',
+                                 b'\xff\xff\x00\xe0'
+                                 b'\x00\x01\x02\x03'
+                                 b'\xfe\xff\xdd\xe0',
+                                 is_undefined_length=True)
+        with pytest.raises(ValueError, match='Pixel Data .* must '
+                                             'start with an item tag'):
+            write_data_element(self.fp, pixel_data)
+
+    def test_big_endian_incorrect_data(self):
+        """Writing pixel data not starting with an item tag raises."""
+        self.fp.is_little_endian = False
+        self.fp.is_implicit_VR = False
+        pixel_data = DataElement(0x7fe00010, 'OB',
+                                 b'\x00\x00\x00\x00'
+                                 b'\x00\x01\x02\x03'
+                                 b'\xff\xfe\xe0\xdd',
+                                 is_undefined_length=True)
+        with pytest.raises(ValueError, match='Pixel Data .+ must '
+                                             'start with an item tag'):
+            write_data_element(self.fp, pixel_data)
+
+
 if __name__ == "__main__":
     # This is called if run alone, but not if loaded through run_tests.py
     # If not run from the directory where the sample images are,