Another NumberOfFrames error pile-up, plus the fuzz target that found it

fuzz/dcm_filehandle_fuzzer.c

@@ -0,0 +1,79 @@
+/*
+ * libdicom fuzz target: parse an arbitrary byte buffer as a DICOM file.
+ *
+ * Exercises the read paths an application hits when it opens an untrusted
+ * DICOM file: File Meta Information, transfer syntax, the metadata data set,
+ * and the first PixelData frame.
+ *
+ * SPDX-License-Identifier: MIT
+ */
+
+#include <stdint.h>
+#include <stdlib.h>
+
+#include <dicom/dicom.h>
+
+int LLVMFuzzerInitialize(int *argc, char ***argv)
+{
+    (void) argc;
+    (void) argv;
+
+    /* Keep the fuzzer quiet; log formatting is not the target here. */
+    dcm_log_set_level(DCM_LOG_NOTSET);
+
+    return 0;
+}
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
+{
+    DcmError *error = NULL;
+    DcmFilehandle *filehandle;
+    DcmDataSet *metadata;
+
+    if (size < 1 || size > (1 << 22)) {
+        return 0;
+    }
+
+    filehandle = dcm_filehandle_create_from_memory(&error,
+                                                   (const char *) data,
+                                                   (int64_t) size);
+    if (filehandle == NULL) {
+        dcm_error_clear(&error);
+        return 0;
+    }
+
+    /* File Meta Information -- the first thing any reader touches.
+     *
+     * Every call clears the error unconditionally: libdicom can return
+     * success with an error still set (see the get_num_frames fix), and a
+     * fuzz target must not turn that into a leak report of its own.
+     */
+    (void) dcm_filehandle_get_file_meta(&error, filehandle);
+    dcm_error_clear(&error);
+
+    (void) dcm_filehandle_get_transfer_syntax_uid(filehandle);
+
+    /* Fast metadata subset, then the full data set. */
+    (void) dcm_filehandle_get_metadata_subset(&error, filehandle);
+    dcm_error_clear(&error);
+
+    metadata = dcm_filehandle_read_metadata(&error, filehandle, NULL);
+    dcm_error_clear(&error);
+    if (metadata != NULL) {
+        dcm_dataset_destroy(metadata);
+    }
+
+    /* Frame offset table + first frame: the encapsulated-pixel-data path. */
+    if (dcm_filehandle_prepare_read_frame(&error, filehandle)) {
+        DcmFrame *frame = dcm_filehandle_read_frame(&error, filehandle, 1);
+
+        if (frame != NULL) {
+            dcm_frame_destroy(frame);
+        }
+    }
+    dcm_error_clear(&error);
+
+    dcm_filehandle_destroy(filehandle);
+
+    return 0;
+}

fuzz/dicom.dict

@@ -0,0 +1,88 @@
+# libFuzzer dictionary for DICOM (PS3.10 file format).
+# Preamble magic, VR codes, item/sequence delimiters, common tags and
+# transfer syntax UIDs -- the tokens a mutator will not discover on its own.
+
+magic_dicm="DICM"
+
+# Value Representations (PS3.5 6.2)
+vr_ae="AE"
+vr_as="AS"
+vr_at="AT"
+vr_cs="CS"
+vr_da="DA"
+vr_ds="DS"
+vr_dt="DT"
+vr_fl="FL"
+vr_fd="FD"
+vr_is="IS"
+vr_lo="LO"
+vr_lt="LT"
+vr_ob="OB"
+vr_od="OD"
+vr_of="OF"
+vr_ol="OL"
+vr_ow="OW"
+vr_pn="PN"
+vr_sh="SH"
+vr_sl="SL"
+vr_sq="SQ"
+vr_ss="SS"
+vr_st="ST"
+vr_tm="TM"
+vr_uc="UC"
+vr_ui="UI"
+vr_ul="UL"
+vr_un="UN"
+vr_ur="UR"
+vr_us="US"
+vr_ut="UT"
+
+# Item and sequence delimitation (FFFE,E000 / E00D / E0DD), little endian
+item_start="\xfe\xff\x00\xe0"
+item_delim="\xfe\xff\x0d\xe0"
+sq_delim="\xfe\xff\xdd\xe0"
+
+# Undefined length
+undef_len="\xff\xff\xff\xff"
+
+# File meta group (0002,xxxx)
+tag_meta_grouplen="\x02\x00\x00\x00"
+tag_meta_version="\x02\x00\x01\x00"
+tag_media_sop_class="\x02\x00\x02\x00"
+tag_media_sop_inst="\x02\x00\x03\x00"
+tag_transfer_syntax="\x02\x00\x10\x00"
+
+# Image description tags (0028,xxxx)
+tag_samples_per_pixel="\x28\x00\x02\x00"
+tag_photometric="\x28\x00\x04\x00"
+tag_number_of_frames="\x28\x00\x08\x00"
+tag_rows="\x28\x00\x10\x00"
+tag_columns="\x28\x00\x11\x00"
+tag_bits_allocated="\x28\x00\x00\x01"
+tag_bits_stored="\x28\x00\x01\x01"
+tag_pixel_repr="\x28\x00\x03\x01"
+
+# Pixel data (7FE0,0010) and float variants
+tag_pixel_data="\xe0\x7f\x10\x00"
+tag_float_pixel_data="\xe0\x7f\x08\x00"
+tag_double_pixel_data="\xe0\x7f\x09\x00"
+
+# Per-frame functional groups / offset table
+tag_perframe_fg="\x37\x52\x11\x91"
+
+# Transfer syntax UIDs
+ts_implicit_le="1.2.840.10008.1.2"
+ts_explicit_le="1.2.840.10008.1.2.1"
+ts_explicit_be="1.2.840.10008.1.2.2"
+ts_deflated="1.2.840.10008.1.2.1.99"
+ts_jpeg_baseline="1.2.840.10008.1.2.4.50"
+ts_jpeg_lossless="1.2.840.10008.1.2.4.70"
+ts_jpeg_ls="1.2.840.10008.1.2.4.80"
+ts_jpeg2000="1.2.840.10008.1.2.4.90"
+ts_jpeg2000_lossy="1.2.840.10008.1.2.4.91"
+ts_rle="1.2.840.10008.1.2.5"
+
+# Photometric interpretations
+pi_mono2="MONOCHROME2"
+pi_rgb="RGB"
+pi_ybr_full_422="YBR_FULL_422"

fuzz/meson.build

@@ -0,0 +1,23 @@
+# libFuzzer targets. Built only when -Dfuzzers=true and the compiler
+# supports -fsanitize=fuzzer, so a normal build is unaffected.
+#
+# Local use:
+#   CC=clang CFLAGS='-fsanitize=fuzzer-no-link' \
+#     meson setup build-fuzz -Dfuzzers=true -Dtests=false \
+#     -Db_sanitize=address,undefined -Db_lundef=false
+#   ninja -C build-fuzz
+#   ./build-fuzz/fuzz/dcm_filehandle_fuzzer fuzz/corpus
+#
+# OSS-Fuzz passes its own engine flags through $LIB_FUZZING_ENGINE.
+
+fuzz_engine = get_option('fuzzing_engine')
+
+foreach target : ['dcm_filehandle_fuzzer']
+  executable(
+    target,
+    target + '.c',
+    dependencies : [libdicom_dep],
+    link_args : fuzz_engine != '' ? [fuzz_engine] : ['-fsanitize=fuzzer'],
+    install : false,
+  )
+endforeach

meson.build

@@ -192,6 +192,10 @@ libdicom_dep = declare_dependency(
 )
 meson.override_dependency('libdicom', libdicom_dep)
 
+if get_option('fuzzers')
+  subdir('fuzz')
+endif
+
 # tools
 executable(
   'dcm-dump',

meson_options.txt

@@ -9,3 +9,15 @@ option(
   value : true,
   description : 'build tests',
 )
+option(
+  'fuzzers',
+  type : 'boolean',
+  value : false,
+  description : 'build libFuzzer fuzz targets (requires a clang toolchain)',
+)
+option(
+  'fuzzing_engine',
+  type : 'string',
+  value : '',
+  description : 'link flag for an external fuzzing engine (OSS-Fuzz: $LIB_FUZZING_ENGINE)',
+)

src/dicom-file.c

@@ -295,7 +295,13 @@ static bool get_num_frames(DcmError **error,
                            uint32_t *number_of_frames)
 {
     const char *value;
-    if (!get_tag_str(error, metadata, "NumberOfFrames", &value)) {
+    /* NumberOfFrames is optional -- pass NULL for error, since a missing
+     * tag is not a failure here. Passing error would leave a stale error
+     * object behind: it is never returned to the caller (so it leaks), and
+     * dcm_error_set() refuses to overwrite an error that is already set, so
+     * every later failure in this call chain would be silently discarded.
+     */
+    if (!get_tag_str(NULL, metadata, "NumberOfFrames", &value)) {
         *number_of_frames = 1;
         return true;
     }