gcc-15 -fanalyzer fixes by gdevenyi · Pull Request #146 · BIC-MNI/libminc

gdevenyi · 2026-04-06T16:30:11Z

Summary

Compiled libminc with gcc-15 -fanalyzer and fixed all genuine bugs it found across 16 files (100 insertions, 12 deletions). All fixes are minimal, internal-only changes — no external interface modifications.

Bug categories fixed:

NULL dereferences after allocation failures (12 fixes): Added missing NULL checks after malloc/calloc/MALLOC in m2util.c, grpattr.c, dimension.c, datatype.c, volume.c, voxel_loop.c, netcdf_convenience.c, hdf_convenience.c, time_stamp.c, input_mnc2.c, input_nifti.c, files.c
Memory leaks on error paths (4 fixes): Fixed partial allocation cleanup in alloc2d(), dimension handle leaks in micopy_dimension()/micreate_dimension(), temp buffer leak in input_nifti.c
Uninitialized variables (4 fixes): Zero-initialized chunk_start, var_end in image_conversion.c; block_start/block_index in volume_cache.c; quoted in files.c; guarded use of uninitialized ch in files.c
Resource leak: FILE handle leak in milog_init() on re-initialization
va_list misuse: Double va_end in v_mi2log_message()
Missing switch defaults: Added default cases to MI_TO_DOUBLE/MI_FROM_DOUBLE macros

Remaining warnings

19 warnings remain, all false positives (e.g., callers always pass valid pointers, loops always fill arrays, static-lifetime resources).

Test plan

All 56 tests pass after every individual commit
Full clean rebuild with -fanalyzer confirms no remaining actionable warnings
ctest --parallel 32 --output-on-failure passes on final state

🤖 Generated with Claude Code

v_mi2log_message() called va_end() on the va_list parameter, but its callers milog_message() and mi2log_message() also called va_end() on the same va_list after the function returned. This is undefined behavior per C standard. Remove the va_end from v_mi2log_message since the callers own the va_list lifecycle. Found by gcc-15 -fanalyzer: -Wanalyzer-va-list-use-after-va-end Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

If milog_init() was called more than once and the log was configured to write to a file (via MINC_LOGFILE), the previously opened FILE* would be leaked. Close the old file handle before opening a new one, being careful not to close stderr or stdout. Found by gcc-15 -fanalyzer: -Wanalyzer-file-leak Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

malloc() can return NULL on allocation failure, but the result was used immediately in strcpy() without a NULL check. Found by gcc-15 -fanalyzer: -Wanalyzer-possible-null-argument Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

When a row allocation failed partway through alloc2d(), previously allocated rows (mat[0]..mat[i-1]) were leaked because only the pointer array was freed. Free all previously allocated rows before freeing the pointer array. Found by gcc-15 -fanalyzer: -Wanalyzer-malloc-leak Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

alloc1d() and alloc2d() can return NULL on allocation failure. Three functions used the result without checking: - scaled_maximal_pivoting_gaussian_elimination: s from alloc1d - scaled_maximal_pivoting_gaussian_elimination_real: row, a, solution - invert_4x4_matrix: mtmp, itmp from alloc2d Add NULL checks with proper cleanup on failure. Found by gcc-15 -fanalyzer: -Wanalyzer-null-dereference Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

hdf_get_diminfo() did not initialize *ndims on error (when H5Dget_space failed), leaving callers with an uninitialized value. Initialize *ndims = 0 on the error path. Also add NULL checks for return values of hdf_dim_add() and hdf_var_add(), which can return NULL when limits are exceeded. Found by gcc-15 -fanalyzer: -Wanalyzer-use-of-uninitialized-value, -Wanalyzer-null-dereference Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Two malloc calls lacked NULL checks: - milist_grp_start: frame allocation used without check, leaking the previously allocated data struct on failure - miset_attr_at_loc: std_name allocation used in strcpy/strlen without check Add NULL checks with proper cleanup on failure paths. Found by gcc-15 -fanalyzer: -Wanalyzer-possible-null-dereference Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

micopy_dimension: error paths leaked handle->name when freeing handle on offsets/widths allocation failure. Also leaked handle->offsets and handle->units in the widths failure path. micreate_dimension: default case freed handle but leaked handle->name and handle->comments. widths allocation failure path also leaked handle->comments. Added NULL check for widths malloc result. miset_apparent_dimension_order and miset_apparent_dimension_order_by_name: dim_indices malloc result not checked before use in memset. Found by gcc-15 -fanalyzer: -Wanalyzer-malloc-leak, -Wanalyzer-possible-null-dereference, -Wanalyzer-possible-null-argument Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Both malloc calls for *name were used without NULL checks - one in strcpy, the other passed to miget_attr_values. Found by gcc-15 -fanalyzer: -Wanalyzer-possible-null-argument Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

malloc result for hdim was passed to memset without a NULL check. Found by gcc-15 -fanalyzer: -Wanalyzer-possible-null-argument Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Multiple MALLOC calls had no NULL checks: - create_loop_options: loop_options struct allocation - create_loop_info: loop_info struct allocation - initialize_loopfile_info: loopfile_info struct and all member arrays (input_files, output_files, output_mincid, output_icvid, input_mincid, input_icvid) - update_history: string allocation for history concatenation Add NULL checks that return NULL or return early on allocation failure. Found by gcc-15 -fanalyzer: -Wanalyzer-possible-null-dereference Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

MALLOC for compfile was used in strcpy without a NULL check. Found by gcc-15 -fanalyzer: -Wanalyzer-possible-null-argument Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- extract_directory: expand_filename can return NULL on allocation failure; add NULL check before using the result - get_absolute_filename: same expand_filename NULL check - input_possibly_quoted_string: 'quoted' was uninitialized when input_nonwhite_character failed, causing unget_character to be called with an uninitialized 'ch'. Initialize quoted=FALSE and guard the unget with status==VIO_OK check Found by gcc-15 -fanalyzer: -Wanalyzer-null-dereference, -Wanalyzer-use-of-uninitialized-value Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

calloc for slice_start was used without a NULL check in miget_slice_range and the subsequent loop. Found by gcc-15 -fanalyzer: -Wanalyzer-possible-null-dereference Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

malloc for temp_buffer was used without a NULL check, and an early return path (volume allocation failure) leaked the buffer. Add a NULL check after malloc and free temp_buffer before the early return. Found by gcc-15 -fanalyzer: -Wanalyzer-possible-null-dereference, -Wanalyzer-malloc-leak Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The MI_TO_DOUBLE macro's switch on type and inner switches on sign had no default cases. If an unexpected type or sign value was encountered, dvalue would remain uninitialized. Add default cases that set dvalue=0 in MI_TO_DOUBLE and no-op defaults in MI_FROM_DOUBLE. This fixes many -Wanalyzer-use-of-uninitialized-value warnings across dim_conversion.c, value_conversion.c, and other files that use these macros. Found by gcc-15 -fanalyzer: -Wanalyzer-use-of-uninitialized-value Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The NULL check for slice_start incorrectly returned VIO_ERROR (an int) from a function returning Minc_file (a pointer). Return NULL instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

chunk_start array was uninitialized if icvp->var_ndims was 0. Zero-initialize the array to prevent use of uninitialized values. Found by gcc-15 -fanalyzer: -Wanalyzer-use-of-uninitialized-value Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

In cache_block_has_data, block_start array could have uninitialized elements if to_volume_index mapped to indices not filled by get_block_start. In get_cache_block, the switch on n_dims had no default case, leaving block_index and block_start uninitialized for unexpected dimension counts. Zero-initialize both variables. Found by gcc-15 -fanalyzer: -Wanalyzer-use-of-uninitialized-value Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

var_end array was uninitialized if icvp->var_ndims was 0, same pattern as chunk_start. Zero-initialize the array. Found by gcc-15 -fanalyzer: -Wanalyzer-use-of-uninitialized-value Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This PR addresses real defects reported by GCC 15 -fanalyzer across libminc, primarily by hardening allocation/error paths, fixing leaks, and initializing previously uninitialized locals to avoid undefined behavior.

Changes:

Added missing allocation-failure checks and related cleanup in multiple IO and libminc components.
Fixed several leak/error-path issues (including log/file handle management) and corrected some va_list handling.
Initialized local arrays/variables and added missing switch defaults to avoid uninitialized reads.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
volume_io/Volumes/volume_cache.c	Initializes cache block locals to avoid uninitialized reads.
volume_io/Volumes/input_nifti.c	Adds temp buffer allocation checks and frees on some error paths while reading NIfTI data.
volume_io/Volumes/input_mnc2.c	Adds allocation failure check for slice-range iteration buffer.
volume_io/Prog_utils/files.c	Handles `expand_filename()` failure and fixes uninitialized/unsafe unget path.
libsrc2/volume.c	Adds malloc NULL-check for dimension handle allocation.
libsrc2/m2util.c	Improves partial-allocation cleanup and adds NULL checks for helper allocations.
libsrc2/grpattr.c	Adds malloc NULL checks in listing/attribute path logic.
libsrc2/dimension.c	Improves cleanup on error paths and adds malloc NULL checks.
libsrc2/datatype.c	Adds malloc NULL checks for space-name retrieval.
libsrc/voxel_loop.c	Adds allocation NULL checks for loop structs and internal arrays.
libsrc/netcdf_convenience.c	Adds malloc NULL check for temporary compressed filename buffer.
libsrc/minc_basic.h	Adds missing `switch` defaults in conversion macros.
libsrc/image_conversion.c	Initializes local coordinate arrays to avoid uninitialized reads.
libsrc/hdf_convenience.c	Adds defensive initialization and NULL checks around HDF5-derived objects.
libcommon/time_stamp.c	Adds malloc NULL check for timestamp string allocation.
libcommon/minc_error.c	Closes previously opened log file on re-init; removes invalid `va_end` on passed-in `va_list`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-06T16:35:06Z


    VIO_Real *temp_buffer = malloc(in_ptr->sizes_in_file[0] *
                                   sizeof(VIO_Real));
+    if (temp_buffer == NULL) return VIO_ERROR;


input_more_nifti_format_file() returns VIO_BOOL, but on temp_buffer allocation failure it returns VIO_ERROR (a VIO_Status), which will likely be treated as TRUE/non-zero and continue with an uninitialized buffer. Return FALSE (and consider setting *fraction_done consistently) on allocation failure.

Suggested change

if (temp_buffer == NULL) return VIO_ERROR;

if (temp_buffer == NULL)

{

if (fraction_done != NULL)

*fraction_done = 0.0;

return FALSE;

}

Copilot · 2026-04-06T16:35:06Z

        slices_count*=dimension_size[d];
      }
      slice_start=(misize_t *)calloc(n_slice_dimensions,sizeof(misize_t));
+      if(slice_start==NULL) return NULL;


On calloc failure for slice_start, the function returns NULL without cleaning up file (allocated via ALLOC(file, 1)) and any fields already allocated/populated earlier (e.g., file->dim_names). Please unwind/close resources before returning (or jump to existing error cleanup) to avoid leaking the partially initialized Minc_file.

Suggested change

if(slice_start==NULL) return NULL;

if(slice_start==NULL)

{

if(file->dim_names != NULL)

{

for_less(d,0,file->n_file_dimensions)

{

if(file->dim_names[d] != NULL)

delete_string(file->dim_names[d]);

}

free(file->dim_names);

}

miclose_volume(file->minc2id);

free(file);

return NULL;

}

Copilot · 2026-04-06T16:35:06Z

  }

  frame = ( struct milistframe * ) malloc ( sizeof ( struct milistframe ) );
+  if ( frame == NULL ) {


If frame allocation fails, grp_id returned by midescend_path() is leaked (no H5Gclose(grp_id)), since data is freed and the function returns. Close grp_id on this error path before returning.

Suggested change

if ( frame == NULL ) {

if ( frame == NULL ) {

H5Gclose ( grp_id );

Copilot · 2026-04-06T16:35:07Z

    volume->dim_indices = ( int * ) malloc ( volume->number_of_dims * sizeof ( int ) );
+    if ( volume->dim_indices == NULL )
+      return ( MI_ERROR );
    memset ( volume->dim_indices, -1, sizeof ( volume->number_of_dims ) );


memset(volume->dim_indices, -1, sizeof(volume->number_of_dims)) uses the size of the int field, not the number of bytes in the allocated array. This only initializes a few bytes and leaves most entries uninitialized. Use volume->number_of_dims * sizeof(*volume->dim_indices) (or a loop) here.

Suggested change

memset ( volume->dim_indices, -1, sizeof ( volume->number_of_dims ) );

memset ( volume->dim_indices, -1,

volume->number_of_dims * sizeof ( *volume->dim_indices ) );

Copilot · 2026-04-06T16:35:07Z

    volume->dim_indices = ( int * ) malloc ( volume->number_of_dims * sizeof ( int ) );
+    if ( volume->dim_indices == NULL )
+      return ( MI_ERROR );
    memset ( volume->dim_indices, -1, sizeof ( volume->number_of_dims ) );


Same memset sizing bug as above: sizeof(volume->number_of_dims) does not cover the allocated dim_indices array. Initialize volume->dim_indices using volume->number_of_dims * sizeof(*volume->dim_indices) (or a loop).

Suggested change

memset ( volume->dim_indices, -1, sizeof ( volume->number_of_dims ) );

for ( i = 0; i < volume->number_of_dims; i++ ) {

volume->dim_indices[i] = -1;

}

Copilot · 2026-04-06T16:35:07Z

   if (num_output_files > 0) {
      loopfile_info->output_files = MALLOC(num_output_files, char *);
+      if (loopfile_info->output_files == NULL) { FREE(loopfile_info); return NULL; }
      for (ifile=0; ifile < num_output_files; ifile++)


If output_files allocation fails, the code frees only loopfile_info but leaks loopfile_info->input_files allocated earlier (and leaves other members potentially uninitialized). Consider initializing the struct members to NULL/MI_ERROR up front and calling cleanup_loopfile_info() (or manually freeing prior allocations) on all early-return error paths.

Copilot · 2026-04-06T16:35:07Z

   loopfile_info->output_mincid = MALLOC(num_files, int);
   loopfile_info->output_icvid = MALLOC(num_files, int);
+   if (loopfile_info->output_mincid == NULL || loopfile_info->output_icvid == NULL) {
+      FREE(loopfile_info); return NULL;


On allocation failure for output_mincid/output_icvid, the code frees only loopfile_info, leaking input_files/output_files (and any other already allocated members). Prefer a single cleanup path (e.g., initialize members then call cleanup_loopfile_info()), or explicitly free previously allocated members before returning.

Suggested change

FREE(loopfile_info); return NULL;

if (loopfile_info->output_mincid != NULL) FREE(loopfile_info->output_mincid);

if (loopfile_info->output_icvid != NULL) FREE(loopfile_info->output_icvid);

if (loopfile_info->output_files != NULL) FREE(loopfile_info->output_files);

if (loopfile_info->input_files != NULL) FREE(loopfile_info->input_files);

FREE(loopfile_info);

return NULL;

Copilot · 2026-04-06T16:35:08Z

   loopfile_info->input_mincid = MALLOC(num_files, int);
   loopfile_info->input_icvid = MALLOC(num_files, int);
+   if (loopfile_info->input_mincid == NULL || loopfile_info->input_icvid == NULL) {
+      FREE(loopfile_info); return NULL;


On allocation failure for input_mincid/input_icvid, the code frees only loopfile_info, leaking earlier allocations such as input_files, output_files, and output_mincid/output_icvid. Use a cleanup path that frees all partially allocated members before returning.

Suggested change

FREE(loopfile_info); return NULL;

if (loopfile_info->input_mincid != NULL) {

FREE(loopfile_info->input_mincid);

}

if (loopfile_info->input_icvid != NULL) {

FREE(loopfile_info->input_icvid);

}

if (loopfile_info->output_mincid != NULL) {

FREE(loopfile_info->output_mincid);

}

if (loopfile_info->output_icvid != NULL) {

FREE(loopfile_info->output_icvid);

}

if (loopfile_info->input_files != NULL) {

FREE(loopfile_info->input_files);

}

if (loopfile_info->output_files != NULL) {

FREE(loopfile_info->output_files);

}

FREE(loopfile_info);

return NULL;

Copilot · 2026-04-06T16:35:08Z

      if (!volume_is_alloced(volume))
      {
        print_error("Failed to allocate volume.\n");
+        free(temp_buffer);
        return FALSE;
      }


temp_buffer is freed when volume_is_alloced() fails, but there are other early returns later in this function (e.g., when nifti_read_buffer() reads fewer bytes) that still return without freeing temp_buffer. Consider refactoring to a single cleanup path so temp_buffer is freed on all failure exits.

gdevenyi and others added 20 commits March 29, 2026 18:04

Fix possible NULL dereference in _miget_file_dimension

8489cd6

malloc result for hdim was passed to memset without a NULL check. Found by gcc-15 -fanalyzer: -Wanalyzer-possible-null-argument Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Fix possible NULL dereference in miexpand_file

750eab4

MALLOC for compfile was used in strcpy without a NULL check. Found by gcc-15 -fanalyzer: -Wanalyzer-possible-null-argument Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Fix possible NULL dereference in input_mnc2.c

e95ad20

calloc for slice_start was used without a NULL check in miget_slice_range and the subsequent loop. Found by gcc-15 -fanalyzer: -Wanalyzer-possible-null-dereference Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Fix return type in input_mnc2.c NULL check

fb39f84

The NULL check for slice_start incorrectly returned VIO_ERROR (an int) from a function returning Minc_file (a pointer). Return NULL instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Fix uninitialized var_end in MI_icv_access

a3b2802

var_end array was uninitialized if icvp->var_ndims was 0, same pattern as chunk_start. Zero-initialize the array. Found by gcc-15 -fanalyzer: -Wanalyzer-use-of-uninitialized-value Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings April 6, 2026 16:30

Copilot started reviewing on behalf of gdevenyi April 6, 2026 16:30 View session

Copilot AI reviewed Apr 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gcc-15 -fanalyzer fixes#146

gcc-15 -fanalyzer fixes#146
gdevenyi wants to merge 20 commits intoBIC-MNI:developfrom
gdevenyi:gcc15-fanalyzer-fixes

gdevenyi commented Apr 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 6, 2026

Uh oh!

Copilot AI Apr 6, 2026

Uh oh!

Copilot AI Apr 6, 2026

Uh oh!

Copilot AI Apr 6, 2026

Uh oh!

Copilot AI Apr 6, 2026

Uh oh!

Copilot AI Apr 6, 2026

Uh oh!

Copilot AI Apr 6, 2026

Uh oh!

Copilot AI Apr 6, 2026

Uh oh!

Copilot AI Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-      if(slice_start==NULL) return NULL;
+      if(slice_start==NULL)
+      {
+        if(file->dim_names != NULL)
+        {
+          for_less(d,0,file->n_file_dimensions)
+          {
+            if(file->dim_names[d] != NULL)
+              delete_string(file->dim_names[d]);
+          }
+          free(file->dim_names);
+        }
+        miclose_volume(file->minc2id);
+        free(file);
+        return NULL;
+      }

	if ( frame == NULL ) {
	if ( frame == NULL ) {
	H5Gclose ( grp_id );

	memset ( volume->dim_indices, -1, sizeof ( volume->number_of_dims ) );
	memset ( volume->dim_indices, -1,
	volume->number_of_dims * sizeof ( *volume->dim_indices ) );

-    memset ( volume->dim_indices, -1, sizeof ( volume->number_of_dims ) );
+    for ( i = 0; i < volume->number_of_dims; i++ ) {
+      volume->dim_indices[i] = -1;
+    }

-      FREE(loopfile_info); return NULL;
+      if (loopfile_info->output_mincid != NULL) FREE(loopfile_info->output_mincid);
+      if (loopfile_info->output_icvid != NULL) FREE(loopfile_info->output_icvid);
+      if (loopfile_info->output_files != NULL) FREE(loopfile_info->output_files);
+      if (loopfile_info->input_files != NULL) FREE(loopfile_info->input_files);
+      FREE(loopfile_info);
+      return NULL;

-      FREE(loopfile_info); return NULL;
+      if (loopfile_info->input_mincid != NULL) {
+         FREE(loopfile_info->input_mincid);
+      }
+      if (loopfile_info->input_icvid != NULL) {
+         FREE(loopfile_info->input_icvid);
+      }
+      if (loopfile_info->output_mincid != NULL) {
+         FREE(loopfile_info->output_mincid);
+      }
+      if (loopfile_info->output_icvid != NULL) {
+         FREE(loopfile_info->output_icvid);
+      }
+      if (loopfile_info->input_files != NULL) {
+         FREE(loopfile_info->input_files);
+      }
+      if (loopfile_info->output_files != NULL) {
+         FREE(loopfile_info->output_files);
+      }
+      FREE(loopfile_info);
+      return NULL;

Conversation

gdevenyi commented Apr 6, 2026

Summary

Bug categories fixed:

Remaining warnings

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants