Repository: pydicom/pydicom

Issue:
LookupError: unknown encoding: Not Supplied
#### Description
Output from `ds = pydicom.read_file(dcmFile)` (an RTSTRUCT dicom file, SOP UID 1.2.840.10008.5.1.4.1.1.481.3) results in some tags throwing a LookupError: "LookupError: unknown encoding: Not Supplied"
Specific tags which cannot be decoded are as follows:
['DeviceSerialNumber',
 'Manufacturer',
 'ManufacturerModelName',
 'PatientID',
 'PatientName',
 'RTROIObservationsSequence',
 'ReferringPhysicianName',
 'SeriesDescription',
 'SoftwareVersions',
 'StructureSetLabel',
 'StructureSetName',
 'StructureSetROISequence',
 'StudyDescription',
 'StudyID']

I suspect that it's due to the fact that `ds.SpecificCharacterSet = 'Not Supplied'`, but when I try to set `ds.SpecificCharacterSet` to something reasonable (ie ISO_IR_100 or 'iso8859'), it doesn't seem to make any difference.

Reading the same file, with NO modifications, in gdcm does not result in any errors and all fields are readable.

#### Steps/Code to Reproduce
```py
import pydicom 
ds = pydicom.read_file(dcmFile)
print(ds.PatientName)
```

#### Expected Results
No error is thrown and the name of the patient is printed.

#### Actual Results
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\Amanda\AppData\Local\Continuum\anaconda3\envs\itk\lib\site-packages\pydicom\valuerep.py", line 706, in __str__
    return '='.join(self.components).__str__()
  File "C:\Users\Amanda\AppData\Local\Continuum\anaconda3\envs\itk\lib\site-packages\pydicom\valuerep.py", line 641, in components
    self._components = _decode_personname(groups, self.encodings)
  File "C:\Users\Amanda\AppData\Local\Continuum\anaconda3\envs\itk\lib\site-packages\pydicom\valuerep.py", line 564, in _decode_personname
    for comp in components]
  File "C:\Users\Amanda\AppData\Local\Continuum\anaconda3\envs\itk\lib\site-packages\pydicom\valuerep.py", line 564, in <listcomp>
    for comp in components]
  File "C:\Users\Amanda\AppData\Local\Continuum\anaconda3\envs\itk\lib\site-packages\pydicom\charset.py", line 129, in decode_string
    return value.decode(encodings[0])
LookupError: unknown encoding: Not Supplied

#### Versions
Platform: Windows-10-10.0.17763-SP0
Python Version: Python 3.6.4 |Anaconda, Inc.| (default, Mar 12 2018, 20:20:50) [MSC v.1900 64 bit (AMD64)]
pydicom Version: pydicom 1.2.2


Discussion:
mrbean-bremen: You are right of course I completely ignored this. I will add that later. 
mrbean-bremen: Hm - I tried that, but this broke other tests. The problem is that `read_encoding` is the encoding the dataset has been read with - changing the value of `Specific Character Set` does not change the encoding which has been used for decoding before. 
It works in simple cases like the given one, as no encoding has been used anyway, but I'm not sure yet how to solve this in the general case. I'll leave this for another day - right now I've caught a cold and have some trouble thinking straight.
darcymason: Of course, this will be a tough one to figure out -- SpecificCharacterSet is one of the very few data elements  that has side-effects on other data elements, and so its complicated to try to carry it along in a dataset.  We need to use it to decode text, but there are different use cases -- correcting it before converting elements, like the original issue, or perhaps people could set it somewhere later.  If I can help sort this out, let me know.
mrbean-bremen: I have looked into this again a bit, and still not sure how to handle this best.
As far as I can see, there are two use cases for changing an existing SpecificCharacterSet:
- the existing character set was used to encode the data, and the new one shall be used if encoding it again (using the cached old one for decoding if not yet done, e.g. for raw data elements)
- the existing character set was incorrect and has to be replaced with a new one

If I assume the second case, as for this issue, I would break the first use case (which has couple of tests). One possibility could be to add a special method, something like `correct_character_set`, that explicitely does the needed stuff, though that would not hinder anyone to not use it... Anyway, as the second case handles incorrect DICOM, while the first is a use case for valid DICOM, I would prefer to have the first case as the default (as it is now).
I'm glad for any suggestion here...
mrbean-bremen: Ping @darcymason, @scaramallion. 
scaramallion: I think I need to get more familiar with the code before I can comment.





darcymason: Generally speaking, I think we should not code for invalid dicom, except in terms of giving clear error messages to users, or perhaps sane defaults with warnings..  For most invalid dicom, I'm in favor of the `pydicom.util.fixer` type of solution like I commented in #820, correcting bad values before use.  Perhaps there is a way to pre-build a `fixer` type solution for the second case.  If you're willing to try it @mrbean-bremen , then we could add something.  But it is perhaps a corner case we don't need to specifically handle.
mrbean-bremen: Ok, thanks. I probably set this to merge and check the `fixer` stuff separately next week (traveling right now). 

Patch:
diff --git a/doc/image_data_handlers.rst b/doc/image_data_handlers.rst
index aa79bcff9..c64f4fe8a 100644
--- a/doc/image_data_handlers.rst
+++ b/doc/image_data_handlers.rst
@@ -56,7 +56,7 @@ handled by the given packages:
 +------------------------------------+------------------------+-------+---------+---------+-----------------+
 | JPEG Lossless (Process 14)         | 1.2.840.10008.1.2.4.57 |       |         | |chk|   |                 |
 +------------------------------------+------------------------+-------+---------+---------+-----------------+
-| JPEG Lossless (Process 14, SV1)    | 1.2.840.10008.1.2.4.70 |       |         | |chk|   |     |chk|       |
+| JPEG Lossless (Process 14, SV1)    | 1.2.840.10008.1.2.4.70 |       |         | |chk|   | |chk|\ :sup:`5` |
 +------------------------------------+------------------------+-------+---------+---------+-----------------+
 | JPEG LS Lossless                   | 1.2.840.10008.1.2.4.80 |       | |chk|   | |chk|   |                 |
 +------------------------------------+------------------------+-------+---------+---------+-----------------+
diff --git a/doc/whatsnew/v1.3.0.rst b/doc/whatsnew/v1.3.0.rst
index 36a0807f9..8ce7efb35 100644
--- a/doc/whatsnew/v1.3.0.rst
+++ b/doc/whatsnew/v1.3.0.rst
@@ -20,6 +20,8 @@ Enhancements
   ``datadict.add_private_dict_entries`` to add custom private tags
   (:issue:`799`)
 * Added possibility to write into zip file using gzip (:issue:`753`)
+* Added handling of incorrect transfer syntax (explicit vs implicit)
+  (:issue:`820`)
 * Added creation of Tag instances by DICOM keyword, e.g Tag("PatientName")
 
 Fixes
@@ -34,4 +36,6 @@ Fixes
 * Fixed `generate_uid()` returning non-conformant UIDs when `prefix=None`
   (:issue:`788`)
 * Avoid exception if reading from empty file (:issue:`810`)
+* An invalid encoding is now replaced by the default encoding, if
+  ``config.enforce_valid_values`` is not set (:issue:`815`)
 * Correctly handle elements with ambiguous VR in sequence items (:issue:`804`)
diff --git a/pydicom/charset.py b/pydicom/charset.py
index 636e5c814..9d11d3f24 100644
--- a/pydicom/charset.py
+++ b/pydicom/charset.py
@@ -125,15 +125,24 @@ def decode_string(value, encodings, delimiters):
     """
     # shortcut for the common case - no escape sequences present
     if ESC not in value:
+        first_encoding = encodings[0]
         try:
-            return value.decode(encodings[0])
+            return value.decode(first_encoding)
+        except LookupError:
+            if config.enforce_valid_values:
+                raise
+            warnings.warn(u"Unknown encoding '{}' - "
+                          u"using default encoding instead"
+                          .format(first_encoding))
+            first_encoding = default_encoding
+            return value.decode(first_encoding)
         except UnicodeError:
             if config.enforce_valid_values:
                 raise
-            warnings.warn(u"Failed to decode byte string with encoding {} - "
+            warnings.warn(u"Failed to decode byte string with encoding '{}' - "
                           u"using replacement characters in decoded "
-                          u"string".format(encodings[0]))
-            return value.decode(encodings[0], errors='replace')
+                          u"string".format(first_encoding))
+            return value.decode(first_encoding, errors='replace')
 
     # Each part of the value that starts with an escape sequence is decoded
     # separately. If it starts with an escape sequence, the
diff --git a/pydicom/filereader.py b/pydicom/filereader.py
index 64ba05a8b..7bada88fc 100644
--- a/pydicom/filereader.py
+++ b/pydicom/filereader.py
@@ -347,6 +347,59 @@ def data_element_generator(fp,
                                      is_implicit_VR, is_little_endian)
 
 
+def _is_implicit_vr(fp, implicit_vr_is_assumed, is_little_endian, stop_when):
+    """Check if the real VR is explicit or implicit.
+
+    Parameters
+    ----------
+    fp : an opened file object
+    implicit_vr_is_assumed : boolean
+        True if implicit VR is assumed.
+        If this does not match with the real transfer syntax, a user warning
+        will be issued.
+    is_little_endian : boolean
+        True if file has little endian transfer syntax.
+        Needed to interpret the first tag.
+    stop_when : None, optional
+        Optional call_back function which can terminate reading.
+        Needed to check if the next tag still belongs to the read dataset.
+
+    Returns
+    -------
+    True if implicit VR is used, False otherwise.
+    """
+    tag_bytes = fp.read(4)
+    vr = fp.read(2)
+    if len(vr) < 2:
+        return implicit_vr_is_assumed
+
+    # it is sufficient to check if the VR is in valid ASCII range, as it is
+    # extremely unlikely that the tag length accidentally has such a
+    # representation - this would need the first tag to be longer than 16kB
+    # (e.g. it should be > 0x4141 = 16705 bytes)
+    vr1 = ord(vr[0]) if in_py2 else vr[0]
+    vr2 = ord(vr[1]) if in_py2 else vr[1]
+    found_implicit = not (0x40 < vr1 < 0x5B and 0x40 < vr2 < 0x5B)
+
+    if found_implicit != implicit_vr_is_assumed:
+        # first check if the tag still belongs to the dataset if stop_when
+        # is given - if not, the dataset is empty and we just return
+        endian_chr = "<" if is_little_endian else ">"
+        tag = TupleTag(unpack(endian_chr + "HH", tag_bytes))
+        if stop_when is not None and stop_when(tag, vr, 0):
+            return found_implicit
+
+        # got to the real problem - warn or raise depending on config
+        found_vr = 'implicit' if found_implicit else 'explicit'
+        expected_vr = 'implicit' if not found_implicit else 'explicit'
+        message = ('Expected {0} VR, but found {1} VR - using {1} VR for '
+                   'reading'.format(expected_vr, found_vr))
+        if config.enforce_valid_values:
+            raise InvalidDicomError(message)
+        warnings.warn(message, UserWarning)
+    return found_implicit
+
+
 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):
@@ -384,12 +437,15 @@ def read_dataset(fp, is_implicit_VR, is_little_endian, bytelength=None,
         A collection (dictionary) of Dicom `DataElement` instances.
     """
     raw_data_elements = dict()
-    fpStart = fp.tell()
+    fp_start = fp.tell()
+    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,
                                     specific_tags)
     try:
-        while (bytelength is None) or (fp.tell() - fpStart < bytelength):
+        while (bytelength is None) or (fp.tell() - fp_start < bytelength):
             raw_data_element = next(de_gen)
             # Read data elements. Stop on some errors, but return what was read
             tag = raw_data_element.tag


Test patch:
diff --git a/pydicom/tests/test_charset.py b/pydicom/tests/test_charset.py
index ff3a5f8e1..2598e6018 100644
--- a/pydicom/tests/test_charset.py
+++ b/pydicom/tests/test_charset.py
@@ -100,6 +100,26 @@ class TestCharset(object):
         ds.decode()
         assert u'CompressedSamples^CT1' == ds.PatientName
 
+    def test_invalid_character_set(self):
+        """charset: replace invalid encoding with default encoding"""
+        ds = dcmread(get_testdata_files("CT_small.dcm")[0])
+        ds.read_encoding = None
+        ds.SpecificCharacterSet = 'Unsupported'
+        with pytest.warns(UserWarning,
+                          match=u"Unknown encoding 'Unsupported' "
+                                u"- using default encoding instead"):
+            ds.decode()
+            assert u'CompressedSamples^CT1' == ds.PatientName
+
+    def test_invalid_character_set_enforce_valid(self):
+        """charset: raise on invalid encoding"""
+        config.enforce_valid_values = True
+        ds = dcmread(get_testdata_files("CT_small.dcm")[0])
+        ds.read_encoding = None
+        ds.SpecificCharacterSet = 'Unsupported'
+        with pytest.raises(LookupError, match='unknown encoding: Unsupported'):
+            ds.decode()
+
     def test_decoding_with_specific_tags(self):
         """Decoding is correctly applied even if  Specific Character Set
         is not in specific tags..."""
@@ -126,8 +146,8 @@ class TestCharset(object):
         elem = DataElement(0x00100010, 'PN',
                            b'\xc4\xe9\xef\xed\xf5\xf3\xe9\xef\xf2')
 
-        with pytest.warns(UserWarning, match='Failed to decode byte string '
-                                             'with encoding UTF8'):
+        with pytest.warns(UserWarning, match="Failed to decode byte string "
+                                             "with encoding 'UTF8'"):
             pydicom.charset.decode(elem, ['ISO_IR 192'])
             assert u'���������' == elem.value
 
@@ -235,9 +255,11 @@ class TestCharset(object):
             # make sure no warning is issued for the correct value
             assert 1 == len(w)
 
-        # not patched incorrect encoding raises
+        # not patched incorrect encoding is replaced by default encoding
         elem = DataElement(0x00100010, 'PN', b'Buc^J\xc3\xa9r\xc3\xb4me')
-        with pytest.raises(LookupError):
+        with pytest.warns(UserWarning,
+                          match=u"Unknown encoding 'ISOIR 192' - "
+                                u"using default encoding instead"):
             pydicom.charset.decode(elem, ['ISOIR 192'])
 
         # Python encoding also can be used directly
diff --git a/pydicom/tests/test_filereader.py b/pydicom/tests/test_filereader.py
index 3105ab5ac..3fd7afbbc 100644
--- a/pydicom/tests/test_filereader.py
+++ b/pydicom/tests/test_filereader.py
@@ -13,6 +13,7 @@ import unittest
 import pytest
 
 import pydicom.config
+from pydicom import config
 from pydicom.dataset import Dataset, FileDataset
 from pydicom.data import get_testdata_files
 from pydicom.filereader import dcmread, read_dataset
@@ -660,8 +661,9 @@ class ReaderTests(unittest.TestCase):
                       b'\x31\x30\x30\x30\x38\x2e\x31\x2e'
                       b'\x32\x00')
         fp = BytesIO(bytestream)
-        ds = dcmread(fp, force=True)
-        self.assertTrue('TransferSyntaxUID' in ds.file_meta)
+        with pytest.warns(UserWarning):
+            ds = dcmread(fp, force=True)
+        assert 'TransferSyntaxUID' in ds.file_meta
 
     def test_no_dataset(self):
         """Test reading no elements or preamble produces empty Dataset"""
@@ -697,6 +699,62 @@ class ReaderTests(unittest.TestCase):
             self.fail('Unexpected EOFError raised')
 
 
+class TestIncorrectVR(object):
+    def setup(self):
+        config.enforce_valid_values = False
+        self.ds_explicit = BytesIO(
+            b'\x08\x00\x05\x00CS\x0a\x00ISO_IR 100'  # SpecificCharacterSet
+            b'\x08\x00\x20\x00DA\x08\x0020000101'  # StudyDate
+        )
+        self.ds_implicit = BytesIO(
+            b'\x08\x00\x05\x00\x0a\x00\x00\x00ISO_IR 100'
+            b'\x08\x00\x20\x00\x08\x00\x00\x0020000101'
+        )
+
+    def teardown(self):
+        config.enforce_valid_values = False
+
+    def test_implicit_vr_expected_explicit_used(self):
+        msg = ('Expected implicit VR, but found explicit VR - '
+               'using explicit VR for reading')
+
+        with pytest.warns(UserWarning, match=msg):
+            ds = read_dataset(
+                self.ds_explicit, is_implicit_VR=True, is_little_endian=True
+            )
+        assert ds.SpecificCharacterSet == 'ISO_IR 100'
+        assert ds.StudyDate == '20000101'
+
+    def test_implicit_vr_expected_explicit_used_strict(self):
+        config.enforce_valid_values = True
+        msg = ('Expected implicit VR, but found explicit VR - '
+               'using explicit VR for reading')
+
+        with pytest.raises(InvalidDicomError, match=msg):
+            read_dataset(
+                self.ds_explicit, is_implicit_VR=True, is_little_endian=True)
+
+    def test_explicit_vr_expected_implicit_used(self):
+        msg = ('Expected explicit VR, but found implicit VR - '
+               'using implicit VR for reading')
+
+        with pytest.warns(UserWarning, match=msg):
+            ds = read_dataset(
+                self.ds_implicit, is_implicit_VR=False, is_little_endian=True
+            )
+        assert ds.SpecificCharacterSet == 'ISO_IR 100'
+        assert ds.StudyDate == '20000101'
+
+    def test_explicit_vr_expected_implicit_used_strict(self):
+        config.enforce_valid_values = True
+        msg = ('Expected explicit VR, but found implicit VR - '
+               'using implicit VR for reading')
+        with pytest.raises(InvalidDicomError, match=msg):
+            read_dataset(
+                self.ds_implicit, is_implicit_VR=False, is_little_endian=True
+            )
+
+
 class TestUnknownVR(object):
     @pytest.mark.parametrize(
         'vr_bytes, str_output',
@@ -742,16 +800,19 @@ class TestUnknownVR(object):
     )
     def test_fail_decode_msg(self, vr_bytes, str_output):
         """Regression test for #791."""
+        # start the dataset with a valid tag (SpecificCharacterSet),
+        # as the first tag is used to check the VR
         ds = read_dataset(
             BytesIO(
-                b'\x08\x00\x01\x00' +
+                b'\x08\x00\x05\x00CS\x0a\x00ISO_IR 100'
+                b'\x08\x00\x06\x00' +
                 vr_bytes +
                 b'\x00\x00\x00\x08\x00\x49'
             ),
             False, True
         )
         msg = (
-            r"Unknown Value Representation '{}' in tag \(0008, 0001\)"
+            r"Unknown Value Representation '{}' in tag \(0008, 0006\)"
             .format(str_output)
         )
         with pytest.raises(NotImplementedError, match=msg):