Prevent crash due to invalid private creator (#1639)

* Prevent crash due to invalid private creator

- issue a warning but don't change the private creator logic
- fixes #1638

* Add private creator tag to warning output

doc/release_notes/v2.4.0.rst

@@ -10,4 +10,4 @@ Fixes
 * Fixed length validation of DS values with maximum length without a leading
   zero (:issue:`1632`)
 * Increased download speed with progress bar for test data (:issue:`1611`)
-
+* Fixed crash due to invalid private creator (:issue:`1638`)

pydicom/datadict.py

@@ -1,7 +1,7 @@
 # Copyright 2008-2018 pydicom authors. See LICENSE file for details.
 # -*- coding: utf-8 -*-
 """Access dicom dictionary information"""
-
+import warnings
 from typing import Tuple, Optional, Dict
 
 from pydicom.config import logger
@@ -553,6 +553,11 @@ def get_private_entry(
             f"Private creator '{private_creator}' not in the private "
             "dictionary"
         ) from exc
+    except TypeError as exc:
+        msg = (f"{tag.private_creator} '{private_creator}' "
+               f"is not a valid private creator")
+        warnings.warn(msg)
+        raise KeyError(msg) from exc
 
     # private elements are usually agnostic for
     # "block" (see PS3.5-2008 7.8.1 p44)
@@ -634,7 +639,7 @@ def private_dictionary_description(tag: TagType, private_creator: str) -> str:
         The tag for the element whose description is being retrieved, in any
         of the forms accepted by :func:`~pydicom.tag.Tag`.
     private_creator : str
-        The name of the private createor.
+        The name of the private creator.
 
     Returns
     -------
@@ -644,6 +649,7 @@ def private_dictionary_description(tag: TagType, private_creator: str) -> str:
     Raises
     ------
     KeyError
-        If the tag is not present in the private dictionary.
+        If the tag is not present in the private dictionary,
+        or if the private creator is not valid.
     """
     return get_private_entry(tag, private_creator)[2]

pydicom/dataelem.py

@@ -650,9 +650,9 @@ def name(self) -> str:
         if self.tag.is_private:
             if self.private_creator:
                 try:
-                    # If have name from private dictionary, use it, but
-                    #   but put in square brackets so is differentiated,
-                    #   and clear that cannot access it by name
+                    # If we have the name from the private dictionary, use it,
+                    # but put it in square brackets to make clear
+                    # that the tag cannot be accessed by that name
                     name = private_dictionary_description(
                         self.tag, self.private_creator
                     )

pydicom/tag.py

@@ -226,6 +226,15 @@ def is_private_creator(self) -> bool:
         """
         return self.is_private and 0x0010 <= self.element < 0x0100
 
+    @property
+    def private_creator(self) -> "BaseTag":
+        """Return the private creator tag for the given tag.
+        The result is meaningless if this is not a private tag.
+
+        .. versionadded:: 2.4
+        """
+        return BaseTag((self & 0xffff0000) | self.element >> 8)
+
 
 def TupleTag(group_elem: Tuple[int, int]) -> BaseTag:
     """Fast factory for :class:`BaseTag` object with known safe (group, elem)

pydicom/tests/test_dataset.py

@@ -1245,6 +1245,21 @@ def test_read_invalid_private_tag_number_as_un(self):
         assert ds[tag2].value == b'\x03\x04'
         assert ds[tag2].VR == 'UN'
 
+    def test_invalid_private_creator(self):
+        # regression test for #1638
+        ds = Dataset()
+        ds.add_new(0x00250010, "SL", [13975, 13802])
+        ds.add_new(0x00250011, "LO", "Valid Creator")
+        ds.add_new(0x00251007, "UN", "foobar")
+        ds.add_new(0x00251107, "UN", "foobaz")
+        msg = (r"\(0025, 0010\) '\[13975, 13802]' "
+               r"is not a valid private creator")
+        with pytest.warns(UserWarning, match=msg):
+            assert (str(ds[0x00251007]) == "(0025, 1007) Private tag data"
+                                           "                    UN: 'foobar'")
+        assert (str(ds[0x00251107]) == "(0025, 1107) Private tag data"
+                                       "                    UN: 'foobaz'")
+
     def test_is_original_encoding(self):
         """Test Dataset.write_like_original"""
         ds = Dataset()

pydicom/tests/test_tag.py

@@ -226,7 +226,7 @@ def test_private(self):
         # Group 0 not private
         assert not BaseTag(0x00000001).is_private
 
-    def test_private_creator(self):
+    def test_is_private_creator(self):
         """Test BaseTag.is_private_creator returns correct values."""
         # Non-private tag
         assert not BaseTag(0x00080010).is_private_creator
@@ -236,6 +236,10 @@ def test_private_creator(self):
         assert BaseTag(0x000900FF).is_private_creator
         assert not BaseTag(0x00090100).is_private_creator
 
+    def test_private_creator(self):
+        assert BaseTag(0x00091000).private_creator == BaseTag(0x00090010)
+        assert BaseTag(0x00292526).private_creator == BaseTag(0x00290025)
+
 
 class TestTag:
     """Test the Tag method."""