Repository: pydicom/pydicom

Issue:
Heuristic for Explicit VR acting in sequence datasets
**Describe the bug**
There is a check to confirm implicit VR by looking for two ascii characters and switching to explicit with a warning (#823).  It was thought this was safe because length in first data elements would not be that large.  However, in sequence item datasets this may not be true.

Noted in google group conversation at https://groups.google.com/forum/#!topic/pydicom/VUmvUYmQxc0 (note that the title of that thread is not correct. that was not the problem).

Test demonstrating it and fix already done - PR to follow shortly.


Discussion:

Patch:
diff --git a/doc/release_notes/v1.4.0.rst b/doc/release_notes/v1.4.0.rst
index 5fbe878ea..0396a1584 100644
--- a/doc/release_notes/v1.4.0.rst
+++ b/doc/release_notes/v1.4.0.rst
@@ -30,6 +30,8 @@ Fixes
   frames when the Basic Offset Table was empty. This may still occur when
   multiple fragments per frame are present for non-JPEG transfer syntaxes or
   where no JPEG EOI/EOC marker is present (:issue:`685`)
+* Fixed possible incorrect switch to explicit VR in sequence items
+  (:issue:`999`)
 
 Enhancements
 ............
diff --git a/pydicom/filereader.py b/pydicom/filereader.py
index 93945a15d..d699a29df 100644
--- a/pydicom/filereader.py
+++ b/pydicom/filereader.py
@@ -322,7 +322,8 @@ def _is_implicit_vr(fp, implicit_vr_is_assumed, is_little_endian, stop_when):
 
 def read_dataset(fp, is_implicit_VR, is_little_endian, bytelength=None,
                  stop_when=None, defer_size=None,
-                 parent_encoding=default_encoding, specific_tags=None):
+                 parent_encoding=default_encoding, specific_tags=None,
+                 at_top_level=True):
     """Return a :class:`~pydicom.dataset.Dataset` instance containing the next
     dataset in the file.
 
@@ -348,6 +349,9 @@ def read_dataset(fp, is_implicit_VR, is_little_endian, bytelength=None,
         Character Set* isn't specified.
     specific_tags : list or None
         See :func:`dcmread` for parameter info.
+    at_top_level: bool
+        If dataset is top level (not within a sequence).
+        Used to turn off explicit VR heuristic within sequences
 
     Returns
     -------
@@ -361,8 +365,9 @@ def read_dataset(fp, is_implicit_VR, is_little_endian, bytelength=None,
     """
     raw_data_elements = dict()
     fp_start = fp.tell()
-    is_implicit_VR = _is_implicit_vr(
-        fp, is_implicit_VR, is_little_endian, stop_when)
+    if at_top_level:
+        is_implicit_VR = _is_implicit_vr(
+            fp, is_implicit_VR, is_little_endian, stop_when)
     fp.seek(fp_start)
     de_gen = data_element_generator(fp, is_implicit_VR, is_little_endian,
                                     stop_when, defer_size, parent_encoding,
@@ -456,11 +461,13 @@ def read_sequence_item(fp, is_implicit_VR, is_little_endian, encoding,
             fp.tell() - 4 + offset, bytes2hex(bytes_read)))
     if length == 0xFFFFFFFF:
         ds = read_dataset(fp, is_implicit_VR, is_little_endian,
-                          bytelength=None, parent_encoding=encoding)
+                          bytelength=None, parent_encoding=encoding,
+                          at_top_level=False)
         ds.is_undefined_length_sequence_item = True
     else:
         ds = read_dataset(fp, is_implicit_VR, is_little_endian, length,
-                          parent_encoding=encoding)
+                          parent_encoding=encoding,
+                          at_top_level=False)
         ds.is_undefined_length_sequence_item = False
         logger.debug("%08x: Finished sequence item" % (fp.tell() + offset,))
     ds.seq_item_tell = seq_item_tell


Test patch:
diff --git a/pydicom/tests/test_filereader.py b/pydicom/tests/test_filereader.py
index 02e234f84..c30a3377d 100644
--- a/pydicom/tests/test_filereader.py
+++ b/pydicom/tests/test_filereader.py
@@ -22,6 +22,7 @@ from pydicom.dataelem import DataElement, DataElement_from_raw
 from pydicom.errors import InvalidDicomError
 from pydicom.filebase import DicomBytesIO
 from pydicom.filereader import data_element_generator
+from pydicom.sequence import Sequence
 from pydicom.tag import Tag, TupleTag
 from pydicom.uid import ImplicitVRLittleEndian
 import pydicom.valuerep
@@ -729,6 +730,35 @@ class TestIncorrectVR(object):
                 self.ds_implicit, is_implicit_VR=False, is_little_endian=True
             )
 
+    def test_seq_item_looks_like_explicit_VR(self):
+        # For issue 999.
+
+        # Set up an implicit VR dataset with a "normal" group 8 tag,
+        # followed by a sequence with an item (dataset) having
+        # a data element length that looks like a potential valid VR
+        ds = Dataset()
+        ds.file_meta = Dataset()
+        ds.file_meta.MediaStorageSOPClassUID = "1.1.1"
+        ds.file_meta.MediaStorageSOPInstanceUID = "2.2.2"
+        ds.is_implicit_VR = True
+        ds.is_little_endian = True
+        ds.SOPClassUID = '9.9.9'  # First item group 8 in top-level dataset
+        seq = Sequence()
+        seq_ds = Dataset()
+        seq_ds.BadPixelImage = b"\3" * 0x5244  # length looks like "DR"
+        seq.append(seq_ds)
+        ds.ReferencedImageSequence = seq
+
+        dbio = DicomBytesIO()
+        ds.save_as(dbio, write_like_original=False)
+
+        # Now read the constructed dataset back in
+        # In original issue, shows warning that has detected what appears
+        # to be Explicit VR, then throws NotImplemented for the unknown VR
+        dbio.seek(0)
+        ds = dcmread(dbio)
+        ds.remove_private_tags()  # forces it to actually parse SQ
+
 
 class TestUnknownVR(object):
     @pytest.mark.parametrize(