What are the assertions of dicom-rs against the imported files?

[qarmin]

I recently tried reading a file to myself and saving it, just to see if the resulting file was identical.

In many cases it turned out to be identical, but not always.

Code which I used:

fn check_file(path: &str) {
    let Ok(file_content) = fs::read(path) else {
        return;
    };
    let cursor = std::io::Cursor::new(file_content.clone());
    let res =
        match from_reader(cursor) {
            Ok(res) => res,
            Err(e) => {
                eprintln!("Error: {}", e);
                return;
            }
        };
    if let Err(e) = dicom_json::to_string(&res) {
        eprintln!("Error: {}", e);
        return;
    }

    let all_items = res.clone().into_iter().collect::<Vec<_>>();
    let hash_map = all_items.into_iter().map(|item| (item.tag(), item)).collect::<std::collections::HashMap<_, _>>();

    let mut item_to_dump = Vec::new();
    if let Err(e) = res.write_all(&mut item_to_dump) {
        eprintln!("Error: {}", e);
        return;
    };
    let res2 = from_reader(std::io::Cursor::new(item_to_dump.clone())).unwrap();
    let all_items2 = res2.clone().into_iter().collect::<Vec<_>>();
    let hash_map2 = all_items2.into_iter().map(|item| (item.tag(), item)).collect::<std::collections::HashMap<_, _>>();
    // assert_eq!(hash_map.len(), hash_map2.len(), "DIFFERENT CONTENT, Different number of items");
    dbg!(hash_map.len());
    for (tag, item) in hash_map {
        let item2 = &hash_map2[&tag];
        // assert_eq!(item.value(), item2.value(), "DIFFERENT CONTENT, tag: {:?}", tag);
    }
    fs::write("a.dcm", &item_to_dump).unwrap();
}

There are 3 places which may panic:

• let res2 = from_reader(std::io::Cursor::new(item_to_dump.clone())).unwrap(); - will panic if cannot read file, saved by dicom-rs
• assert_eq!(hash_map.len(), hash_map2.len(), "DIFFERENT CONTENT, Different number of items"); - will panic if there is tag mismatch
• assert_eq!(item.value(), item2.value(), "DIFFERENT CONTENT, tag: {:?}", tag); - will panic if tags have different content

For first item I found this file - compressed.zip
which properly loads at first time, but after saving via dicom-rs, fails to load

called `Result::unwrap()` on an `Err` value: ParseMetaDataSet { source: DecodeElement { source: ReadHeaderTag { backtrace: None, source: Error { kind: UnexpectedEof, message: "failed to fill whole buffer" } } } }

For second item(removed tag) I found this file - compressed.zip

assertion `left == right` failed: DIFFERENT CONTENT, Different number of items
  left: 1
 right: 0

For third item, I found such files - compressed.zip

assertion `left == right` failed: DIFFERENT CONTENT, tag: Tag(0x0001, 0x0001)
  left: Sequence(DataSetSequence { items: [InMemDicomObject { entries: {Tag(0x0001, 0x0001): DataElement { header: DataElementHeader { tag: Tag(0x0001, 0x0001), vr: SQ, len: Length(Undefined) }, value: Sequence(DataSetSequence { items: [InMemDicomObject { entries: {Tag(0x0001, 0x0001): DataElement { header: DataElementHeader { tag: Tag(0x0001, 0x0001), vr: UN, len: Length(16) }, value: Primitive(U8([68, 111, 117, 98, 108, 101, 32, 78, 101, 115, 116, 101, 100, 32, 83, 81])) }}, dict: StandardDataDictionary, len: Length(Undefined), charset_changed: false }], length: Length(Undefined) }) }, Tag(0x0001, 0x0002): DataElement { header: DataElementHeader { tag: Tag(0x0001, 0x0002), vr: UN, len: Length(9) }, value: Primitive(U8([78, 101, 115, 116, 101, 100, 32, 83, 81])) }}, dict: StandardDataDictionary, len: Length(Undefined), charset_changed: false }], length: Length(Undefined) })
 right: Sequence(DataSetSequence { items: [InMemDicomObject { entries: {Tag(0x0001, 0x0001): DataElement { header: DataElementHeader { tag: Tag(0x0001, 0x0001), vr: SQ, len: Length(Undefined) }, value: Sequence(DataSetSequence { items: [InMemDicomObject { entries: {Tag(0x0001, 0x0001): DataElement { header: DataElementHeader { tag: Tag(0x0001, 0x0001), vr: UN, len: Length(16) }, value: Primitive(U8([68, 111, 117, 98, 108, 101, 32, 78, 101, 115, 116, 101, 100, 32, 83, 81])) }}, dict: StandardDataDictionary, len: Length(Undefined), charset_changed: false }], length: Length(Undefined) }) }, Tag(0x0001, 0x0002): DataElement { header: DataElementHeader { tag: Tag(0x0001, 0x0002), vr: UN, len: Length(10) }, value: Primitive(U8([78, 101, 115, 116, 101, 100, 32, 83, 81, 0])) }}, dict: StandardDataDictionary, len: Length(Undefined), charset_changed: false }], length: Length(Undefined) })

or

assertion `left == right` failed: DIFFERENT CONTENT, tag: Tag(0x0008, 0x0016)
  left: Primitive(Strs(["1.2.840.1000\0\0\0\\377\\331\0\\376\\377\\335\\340\0\0\0\0"]))
 right: Primitive(Strs(["1.2.840.1000\0\0\0", "377", "331\0", "376", "377", "335", "340\0\0\0\0"]))

or

assertion `left == right` failed: DIFFERENT CONTENT, tag: Tag(0x0008, 0x0018)
  left: Primitive(Strs(["1.2.826.0.1.3680043.8.498.49043964482360854182530167603\0\u{7}\u{1}US\u{2}\0\\377\\377"]))
 right: Primitive(Strs(["1.2.826.0.1.3680043.8.498.49043964482360854182530167603\0\u{7}\u{1}US\u{2}\0", "377", "377"]))

Are these bugs, or does this library allow you to remove changing some of the tags on import/save?

[Enet4]

The library does not guarantee that reading a DICOM file and writing it back will result in an identical copy of the original file. Depending on some factors, it may choose to change file meta information based on the implementation and tweak elements such as re-encode data element lengths (e.g. rounding to the next even, or going from defined length sequence to undefined length SQs).

On the other hand, that sample code exhibits at least two concerns:

• The object reader may be making certain assumptions about the input which result in a suboptimal reading in the face of unusual input, although it sounds to me that most of those files are invalid DICOM. We can always try to make the reader more resilient, but in the face of invalid input, it is hard to make any assurance about the subsequent output.
• The write_all method call should semantically result in a file with the equivalent element data as the one represented by the DICOM object in memory, and if this provides an invalid DICOM file, then it is likely a bug.

My quick analysis of those files:

• In file 1, this could be dicom_object not being able to produce invalid input when the object in memory contain invalid meta information.
• File 2 might also have incorrect file meta information, leading to other DICOM file writing mistakes.
• I could not find the data reported by some of the error messages for file 3, but there we have 1) an odd-lengthed element that was re-encoded to be padded to even length, which is expected and fine; 2) a data element value which was probably encoded with the wrong length, leading to more decoding mistakes along the way.

I'm not sure if this answers the question, but we can try to narrow the problem down a bit more to specific cases and see what can be done better in those. I greatly appreciate your effort so far in finding these cases, so that the software can be more robust over time.

[qarmin]

Let's pretend that we have this states:

A - initial file content (a.dcm)
B - loaded dicom file to memory via dicom-rs (content of a.dcm)
C - saved object from B to file via dicom-rs (b.dcm)
D - loaded dicom file to memory via dicom-rs (content of b.dcm)

As a library user, I would expect this behavior of the library

• A and C may differ - dcm files may have been created by other programs and dicom does not enforce this, so here it is ok
• if B successfully loads file, then C should always pass - there is a problem now, because some files from the dicom-rs write correctly to file and cannot be read later by same version of library. It looks like the validations/conversions are not identical when loading files and saving them
• B and D should be identical - if I load data from a file from the same version of dicom-rs, save it to a new one and then reload it from the new one, I expect to have two of the same structure. Now, this sometimes differs, probably also due to the difference in validation/conversion when saving loading.
• Overridden tags shouldn't be lost on save/load - I'm not sure about that, but I associate that dicom allows you to save tags outside a certain range. I have seen something like that in some files. Perhaps the current behavior of dicom-rs with deletion of some tags is ok, but at first glance it seemed strange to me.

Here is big pack of problematic files found and minimized by fuzzer -
REPORTS___DICOM.zip

[Enet4]

Thank you for the extensive summary! The tree first points make sense. The fourth point is probably a failed attempt at reading the entirety of the DICOM data for other reasons, which affects subsequent reads and writes. dicom_object does not actively choose to drop data elements when opening a file (maybe the only exception would be when reading the file meta information group, which has a fixed set of supported fields).

There is one thing that intrigues me though, because we already have a fuzz test target for dicom_object with an equivalent workflow: it reads data as DICOM, writes it to bytes, and reads it back to check that the two objects are equivalent. But running it on my machine does not yield any outputs, so I suspect that the fuzzer is being suboptimal at finding issues. Would you have any recommendations on running and tuning the fuzzer for this project?

[qarmin]

I use in fuzzer(also cargo-fuzz) the corpus filled with dcm files, because it greatly reduce time to find problem.

Here is CI with it - https://github.com/qarmin/Automated-Fuzzer/blob/4d563f9ddc031e3b4a654e5fd5959ec31b9cd23d/.github/workflows/cargo_fuzz.yml#L121

In fuzzer I return Corpus enum item to help fuzzer to use operate mostly on valid/almost valid files - https://github.com/qarmin/Automated-Fuzzer/blob/master/fuzz/fuzz_targets/dicom.rs

[Enet4]

OK, feeding the problematic files in REPORTS__DICOM.zip to the fuzzer allowed me to produce new artifacts. This shows clearly a case where state B is not equal to state D: if A contains a data element with an odd-length value (e.g. length = 9 bytes), DICOM-rs will accept it while assuming that the value was encoded with that exact length, even though it is not compliant. dicom_object introduces padding to even length only upon writing, thus creating a difference between the two objects.

I don't exclude the existence of other issues, but this one in particular is one of those "all bets are off" scenarios for which we should not treat B != D as a bug. I tried my best consulting the standard for ideas on how to handle nonconforming data of this form, but ended up empty handed. At best, we could have a way to configure the parser with different strategies when finding data elements with an odd length:

• assume the length as encoded and continue normally (current behavior)
• assume the next even as the length (dcmtk has this too)
• strict mode, do not continue and raise an error

This can be made into a data set reader option, and then at dicom_object we can also offer options based on these, including the option to automatically pad values as they are stored in the dictionary.