Merge pull request #52 from jcupitt/fix-vm-zero

Fix vm zero

Author: jcupitt

meson.build

@@ -8,7 +8,7 @@ project(
   ],
   license : 'MIT',
   meson_version : '>=0.50',
-  version : '0.2.1',
+  version : '0.3.0'
 )
 if not meson.is_subproject()
   meson.add_dist_script(

src/dicom-data.c

@@ -216,13 +216,13 @@ void dcm_element_destroy(DcmElement *element)
 
 uint16_t dcm_element_get_group_number(const DcmElement *element)
 {
-    return (uint16_t)(element->tag >> 16);
+    return element->tag >> 16;
 }
 
 
 uint16_t dcm_element_get_element_number(const DcmElement *element)
 {
-    return (uint16_t)(element->tag);
+    return element->tag & 0xffff;
 }
 
 
@@ -1141,6 +1141,8 @@ void dcm_element_print(const DcmElement *element, int indentation)
                keyword,
                dcm_dict_str_from_vr(element->vr));
     } else {
+        // private tag, or unknown public tag
+	// in any case, we can't display the keyword
         printf("%*.*s (%04X,%04X) | %s",
                num_indent,
                num_indent,

src/dicom-dict.c

@@ -304,6 +304,7 @@ static const struct _DcmAttribute attribute_table[] = {
     {0X00041512, DCM_VR_TAG_UI, "ReferencedTransferSyntaxUIDInFile"},
     {0X0004151A, DCM_VR_TAG_UI, "ReferencedRelatedGeneralSOPClassUIDInFile"},
     {0X00041600, DCM_VR_TAG_UL, "NumberOfReferences"},
+    {0X00080000, DCM_VR_TAG_UL, "GenericGroupLength"},
     {0X00080001, DCM_VR_TAG_UL, "LengthToEnd"},
     {0X00080005, DCM_VR_TAG_CS, "SpecificCharacterSet"},
     {0X00080006, DCM_VR_TAG_SQ, "LanguageCodeSequence"},
@@ -523,6 +524,7 @@ static const struct _DcmAttribute attribute_table[] = {
     {0X00089458, DCM_VR_TAG_SQ, "FrameDisplaySequence"},
     {0X00089459, DCM_VR_TAG_FL, "RecommendedDisplayFrameRateInFloat"},
     {0X00089460, DCM_VR_TAG_CS, "SkipFrameRangeFlag"},
+    {0X00090010, DCM_VR_TAG_LO, "PrivateCreator"},
     {0X00100010, DCM_VR_TAG_PN, "PatientName"},
     {0X00100020, DCM_VR_TAG_LO, "PatientID"},
     {0X00100021, DCM_VR_TAG_LO, "IssuerOfPatientID"},
@@ -5280,12 +5282,20 @@ static const struct _DcmAttribute *attribute_from_tag(uint32_t tag)
 
     dcm_init();
 
+    // tags with element number 0 are generic group length tags ... map all of
+    // these (except 0000,0000) to tag 0008,0000 (GenericGroupLength)
+    if (tag != 0 &&
+        (tag & 0xffff) == 0) {
+        tag = 0x00080000;
+    }
+
     HASH_FIND_INT(attribute_from_tag_dict, &tag, entry);
 
     return (const struct _DcmAttribute *)entry;
 }
 
 
+// this will also fail for unknown or retired public tags
 bool dcm_is_public_tag(uint32_t tag)
 {
     return attribute_from_tag(tag) != NULL;
@@ -5311,25 +5321,32 @@ DcmVR dcm_vr_from_tag(uint32_t tag)
 {
     const struct _DcmAttribute *attribute;
 
-    if ((attribute = attribute_from_tag(tag)) &&
-        dcm_dict_str_from_vr((DcmVR) attribute->vr_tag)) {
-        return (DcmVR) attribute->vr_tag;
+    if (!(attribute = attribute_from_tag(tag))) {
+        return DCM_VR_ERROR;
     }
 
-    return DCM_VR_ERROR;
+    return (DcmVR) attribute->vr_tag;
 }
 
 
 bool dcm_is_valid_vr_for_tag(DcmVR vr, uint32_t tag)
 {
-    const struct _DcmAttribute *attribute = attribute_from_tag(tag);
+    // always fail for illegal VRs
+    if (vr < 0 || vr >= DCM_VR_LAST) {
+        return false;
+    }
 
+    // private tags are unknown to us and can have any legal VR
+    if (dcm_is_private_tag(tag)) {
+        return true;
+    }
+
+    const struct _DcmAttribute *attribute = attribute_from_tag(tag);
     if (attribute == NULL) {
-        // unknown tag
-        return false;
-    } else if (vr < 0 || vr >= DCM_VR_LAST) {
-        // not a VR
-        return false;
+        // unknown public tag ... we don't include retired tags in our 
+        // dictionary, so we can't check them, but we don't want to fail 
+        // for them either
+        return true;
     } else if (vr == (DcmVR) attribute->vr_tag) {
         // trivially equal
         return true;

src/dicom-file.c

@@ -353,12 +353,15 @@ static DcmElement *read_element_header(DcmError **error,
 
     DcmVR vr;
     if (implicit) {
+        // this can be an ambiguious VR, eg. pixeldata is allowed in implicit
+	// mode and has to be disambiguated later from other tags
         vr = dcm_vr_from_tag(tag);
         if (vr == DCM_VR_ERROR) {
             dcm_error_set(error, DCM_ERROR_CODE_PARSE,
                           "Reading of Data Element header failed",
                           "Tag %08X not allowed in implicit mode", tag);
         }
+
         if (!read_uint32(error, filehandle, length, position)) {
             return NULL;
         }
@@ -374,7 +377,7 @@ static DcmElement *read_element_header(DcmError **error,
         if (!dcm_is_valid_vr_for_tag(vr, tag)) {
             dcm_error_set(error, DCM_ERROR_CODE_PARSE,
                           "Reading of Data Element header failed",
-                          "Unknown or mismatched VR %s", vr_str);
+                          "Tag %08X cannot have VR '%s'", tag, vr_str);
             return NULL;
         }
 

src/dicom.c

@@ -26,7 +26,18 @@
 
 void *dcm_calloc(DcmError **error, size_t n, size_t size)
 {
-    void *result = calloc(n, size);
+    /* malloc(0) behaviour depends on the platform heap implementation. It can 
+     * return either a valid pointer that can't be dereferenced, or NULL.
+     *
+     * We need to be able to support dcm_calloc(0), since VM == 0 is allowed,
+     * but we can't return NULL in this case, since that's how we detect
+     * out of memory.
+     *
+     * Instead, force n == 0 to n == 1. This means we will always get a valid
+     * pointer from calloc, a NULL return always means out of memory, and we 
+     * can always free the result.
+     */
+    void *result = calloc(n == 0 ? 1 : n, size);
     if (!result) {
         dcm_error_set(error, DCM_ERROR_CODE_NOMEM,
                       "Out of memory",

tests/check_dicom.c

@@ -294,6 +294,27 @@ START_TEST(test_element_CS_multivalue)
 END_TEST
 
 
+START_TEST(test_element_CS_multivalue_empty)
+{
+    uint32_t tag = 0x00080008;
+    uint32_t vm = 0;
+
+    char **values = malloc(vm * sizeof(char *));
+
+    DcmElement *element = dcm_element_create(NULL, tag, DCM_VR_CS);
+    (void) dcm_element_set_value_string_multi(NULL, element, values, vm, true);
+
+    ck_assert_int_eq(dcm_element_get_tag(element), tag);
+    ck_assert_int_eq(dcm_element_get_vr(element), DCM_VR_CS);
+    ck_assert_int_eq(dcm_element_get_length(element),
+                     compute_length_of_string_value_multi(values, vm));
+    ck_assert_int_eq(dcm_element_is_multivalued(element), false);
+
+    dcm_element_destroy(element);
+}
+END_TEST
+
+
 START_TEST(test_element_DS)
 {
     uint32_t tag = 0x0040072A;
@@ -474,6 +495,56 @@ START_TEST(test_element_US)
 END_TEST
 
 
+START_TEST(test_element_US_multivalue)
+{
+    uint32_t tag = 0x00280010;
+    uint16_t value[] = {512, 513, 514, 515};
+    uint32_t vm = sizeof(value) / sizeof(value[0]);
+
+    DcmElement *element = dcm_element_create(NULL, tag, DCM_VR_US);
+    (void) dcm_element_set_value_numeric_multi(NULL, 
+        element, (int*) value, vm, false);
+
+    ck_assert_int_eq(dcm_element_get_tag(element), tag);
+    ck_assert_int_eq(dcm_element_get_vr(element), DCM_VR_US);
+    ck_assert_int_eq(dcm_element_get_length(element), sizeof(value));
+    ck_assert_int_eq(dcm_element_is_multivalued(element), true);
+
+    for (uint32_t i = 0; i < vm; i++) {
+        int64_t integer;
+        (void) dcm_element_get_value_integer(NULL, element, i, &integer);
+        ck_assert_int_eq(value[i], integer);
+    }
+
+    dcm_element_print(element, 0);
+
+    dcm_element_destroy(element);
+}
+END_TEST
+
+
+START_TEST(test_element_US_multivalue_empty)
+{
+    uint32_t tag = 0x00280010;
+    uint16_t value[] = {};
+    uint32_t vm = sizeof(value) / sizeof(value[0]);
+
+    DcmElement *element = dcm_element_create(NULL, tag, DCM_VR_US);
+    (void) dcm_element_set_value_numeric_multi(NULL, 
+        element, (int*) &value, vm, false);
+
+    ck_assert_int_eq(dcm_element_get_tag(element), tag);
+    ck_assert_int_eq(dcm_element_get_vr(element), DCM_VR_US);
+    ck_assert_int_eq(dcm_element_get_length(element), sizeof(value));
+    ck_assert_int_eq(dcm_element_is_multivalued(element), false);
+
+    dcm_element_print(element, 0);
+
+    dcm_element_destroy(element);
+}
+END_TEST
+
+
 START_TEST(test_sequence)
 {
     DcmElement *element;
@@ -738,13 +809,16 @@ static Suite *create_data_suite(void)
     tcase_add_test(element_case, test_element_AE);
     tcase_add_test(element_case, test_element_AS);
     tcase_add_test(element_case, test_element_CS_multivalue);
+    tcase_add_test(element_case, test_element_CS_multivalue_empty);
     tcase_add_test(element_case, test_element_DS);
     tcase_add_test(element_case, test_element_IS);
     tcase_add_test(element_case, test_element_ST);
     tcase_add_test(element_case, test_element_SQ);
     tcase_add_test(element_case, test_element_SQ_empty);
     tcase_add_test(element_case, test_element_UI);
     tcase_add_test(element_case, test_element_US);
+    tcase_add_test(element_case, test_element_US_multivalue);
+    tcase_add_test(element_case, test_element_US_multivalue_empty);
     suite_add_tcase(suite, element_case);
 
     TCase *dataset_case = tcase_create("dataset");