Ensure the buffer provided to MPAS_io_get_var_generic is large enough. #1367
Conversation
jim-p-w
force-pushed
the
atmosphere/check_buffer_len
branch
from
3d97db2 to
d610024
Compare
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
force-pushed
the
atmosphere/check_buffer_len
branch
from
d610024 to
7f2f3a8
Compare
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.
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
| io_global_err = local_ierr | ||
| if (present(ierr)) ierr = local_ierr | ||
| return | ||
| endif |
There was a problem hiding this comment.
A blank line below this endif might be helpful in providing a visual indicator that the block below is independent of this block that is being terminated here.
| !> Run these tests with valgrind to ensure there are no buffer overflows when | ||
| !> attempting to read strings into undersized buffers. | ||
| !----------------------------------------------------------------------- | ||
| subroutine test_read_string_buffer_check(domain, threadErrs, ierr) |
There was a problem hiding this comment.
With the GCC compilers and using the -Wall flag, there's a warning that threadErrs is never set:
mpas_test_core_io.F:61:62:
61 | subroutine test_read_string_buffer_check(domain, threadErrs, ierr)
| 1
Warning: Dummy argument 'threaderrs' at (1) was declared INTENT(OUT) but was not set [-Wunused-dummy-argument]
We could either ensure that this intent(out) argument is set, or -- since it doesn't appear to be used by any calling code -- we could just eliminate this argument.
| message = 'Length of string variable "'//trim(fieldname)//'" in file "'//trim(handle % filename)//'"' | ||
| IO_ERROR_WRITE(message) | ||
| message = ' exceeds buffer size: len('//trim(fieldname)//')=$i, len(buffer)=$i' | ||
| IO_ERROR_WRITE(message COMMA intArgs=(/dimsizes(1), len(charVal)/)) |
There was a problem hiding this comment.
With the GCC 12.4.0 compilers, this macro seems to work as expected, and I get a message like
ERROR: Length of string variable "xtime" in file "x1.10242.init.nc"
ERROR: exceeds buffer size: len(xtime)=128, len(buffer)=64
in the log file for a test that I've been using.
However, with the Intel oneAPI 2025.1.0 compilers, I get
ERROR: Length of string variable "xtime" in file "x1.10242.init.nc"
ERROR: exceeds buffer size: len(xtime)=**, len(buffer)=**
The problem seems to be in the macro expansion with the ifx compiler. Using the -preprocess-only option, it appears the ifx compiler is expanding the macro as:
call mpas_log_write( message, messageType=MPAS_LOG_ERR)Could you investigate whether there's a workaround that we can employ so that this works with the ifx compiler?
3 participants
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:
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).