Skip to content

Conversation

@jim-p-w
Copy link
Contributor

@jim-p-w jim-p-w commented Oct 14, 2025

This PR fixes a potential buffer overrun when reading string variables from a netcdf file.

A fixed size array is provided as an output buffer when reading a 0d-char character variable.
Call MPAS_io_inq_var prior to the read to get the size of the variable, and only proceed with the read if the size of the variable will fit in the provided array.
Return an error code if the variable value is larger than the provided output buffer.

A unit test is included to verify:

  1. An attempt to read a string variable into a buffer which is too small to hold the string value is detected
  2. When a buffer which is too small is detected, the read won't occur and an error code is returned
  3. When the test is run via valgrind, valgrind detects no memory errors or corruption
  4. If the code which detects the too small buffer is commented out the test crashes and valgrind reports memory corruption

Fixes issue #1350

Note

When building with PIO, if the charArray (or the charArray1d) value exceeds the size of the tempchar buffer provided to the call to PIO_get_var, the value will be truncated to the size of the provided tempchar buffer (lines 2018, 2023, and 2042 in src/framework/mpas_io.F).

@jim-p-w jim-p-w marked this pull request as draft October 14, 2025 22:29
@mgduda mgduda requested review from amstokely and mgduda October 15, 2025 15:51
@jim-p-w jim-p-w force-pushed the atmosphere/check_buffer_len branch from 3d97db2 to d610024 Compare October 16, 2025 16:14
@jim-p-w jim-p-w marked this pull request as ready for review October 16, 2025 16:48
A fixed size array is provided as an output buffer when reading a 0d-char
character variable. Call MPAS_io_inq_var prior to the read to get the
size of the variable, and only proceed with the read if the size of the
variable will fit in the provided array.
Return an error code if the variable value is larger than the provided
output buffer.
1. Fix Makefile
2. White space changes
3. Use size() function for loop upper bound
4. Fix typos
5. Copyright change
@jim-p-w jim-p-w force-pushed the atmosphere/check_buffer_len branch from d610024 to 7f2f3a8 Compare December 22, 2025 21:13
1. change MPAS_IO_ERR_INSUFFICIENT_ARG to MPAS_IO_ERR_INSUFFICIENT_BUF
   and update the corresponding message to be variable type agnostic.
2. fix whitespace and identifier capitalization.
if (local_ierr == MPAS_IO_ERR_INSUFFICIENT_BUF) then
call MPAS_io_err_mesg(handle % ioContext, local_ierr, .false.)
io_global_err = local_ierr
if (present(ierr)) ierr = local_ierr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of an MPAS_IO_ERR_INSUFFICIENT_BUF error, this will be the last statement executed in this subroutine, and there may be an opportunity to minimize the changes to the code below this block. Rather than logic that is structured like

      if (local_ierr == MPAS_IO_ERR_INSUFFICIENT_ARG) then
         ... code ...
      else
#ifdef MPAS_PIO_SUPPORT
        if (pio_ierr /= PIO_noerr) then
           ... code ...
           return
        end if
#endif

#ifdef MPAS_SMIOL_SUPPORT
        if (local_ierr /= SMIOL_SUCCESS) then
           ... code ...
           return
        end if
#endif
      end if

what if we used something like this:

      if (local_ierr == MPAS_IO_ERR_INSUFFICIENT_ARG) then
         ... code ...
         return
      end if

#ifdef MPAS_PIO_SUPPORT
      if (pio_ierr /= PIO_noerr) then
         ... code ...
         return
      end if
#endif

#ifdef MPAS_SMIOL_SUPPORT
      if (local_ierr /= SMIOL_SUCCESS) then
         ... code ...
         return
      end if
#endif

The idea here is that everything from the #ifdef MPAS_PIO_SUPPORT line downward is completely unchanged from the base code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice observation. Done.

module mpas_io

#define IO_DEBUG_WRITE(M, ARGS) !call mpas_log_write(M, ARGS)
#define IO_ERROR_WRITE(M, ARGS) call mpas_log_write( M, ARGS, messageType=MPAS_LOG_ERR)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that we don't need to make up an unused argument in cases where we don't have any arguments to the mpas_log_write call, for example:

IO_ERROR_WRITE('Here is a message', intArgs=[0])

we could instead define this macro as:

#define IO_ERROR_WRITE(M) call mpas_log_write(M, messageType=MPAS_LOG_ERR)

In order to allow for the passing of other arguments to mpas_log_write, we can also define the macro COMMA as:

#define COMMA ,

Then, an error message with arguments would look something like:

IO_ERROR_WRITE('The answer is $i' COMMA intArgs=[42])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever! I had wondered about the definition of COMMA in other files. Done.

if (dimsizes(1) > len(charVal)) then
local_ierr = MPAS_IO_ERR_INSUFFICIENT_BUF
message = 'MPAS_io_get_var_generic var "'//trim(fieldname)// &
'" len too big, len=$i buflen=$i'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make this message more helpful to users if we used something like:

            message = 'Length of string variable "'//trim(fieldname)//'" in file "'//trim(handle % filename)//'"'
            IO_ERROR_WRITE(message, intArgs=(/0/))
            message = '    exceeds buffer size: len('//trim(fieldname)//')=$i, len(buffer)=$i'
            IO_ERROR_WRITE(message, intArgs=(/dimsizes(1), len(charVal)/)) 

This would generate an error log entry something like:

ERROR: Length of string variable "xtime" in file "x1.10242.init.nc"
ERROR:     exceeds buffer size: len(xtime)=128, len(buffer)=64

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I love printing out the filename which contains the variable. Done.

1. Improve error message in MPAS_io_get_var_generic
2. Make IO_DEBUG_WRITE and IO_ERROR_WRITE macros more flexible
3. Improve readability of MPAS_io_get_var_generic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants