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
1. remove unused argument from test_core_io_test, test_read_string_buffer_check 2. change debug log macros to work with intel compiler
src/core_test/mpas_test_core_io.F
Outdated
|
|
||
| ! log an error message | ||
| if (present(message)) then | ||
| if (present(args)) then |
There was a problem hiding this comment.
I think optional arguments can be passed through to other subroutines whose corresponding dummy arguments are also optional without any need to check presence. So it may be possible to simplify this block of code as:
if (present(message)) then
ERROR_WRITE_ARGS(message, intArgs=args)
end if
src/core_test/mpas_test_core_io.F
Outdated
| integer :: local_ierr, i | ||
| type(MPAS_IO_Handle_type) :: fileHandle | ||
| character (len=StrKIND), dimension(1), parameter :: dimNamesString = ['StrLen'] | ||
| character (len=StrKIND), dimension(2), parameter :: dimNamesStringTime = ['StrLen', 'Time '] |
There was a problem hiding this comment.
Although it apparently does no harm, are the two spaces in the 'Time ' dimension intentional?
There was a problem hiding this comment.
Ok, I see there's a compilation error if these strings don't have the same length:
mpas_test_core_io.F:65:84:
65 | character (len=StrKIND), dimension(2), parameter :: dimNamesStringTime = ['StrLen', 'Time']
| 1
Error: Different CHARACTER lengths (6/4) in array constructor at (1)
It should work to declare dimNamesStringTime as:
character (len=StrKIND), dimension(2), parameter :: dimNamesStringTime = [character(len=StrKIND) :: 'StrLen', 'Time']There was a problem hiding this comment.
Thanks, I wouldn't have figured that out!
src/core_test/mpas_test_core_io.F
Outdated
| ierr = 0 | ||
|
|
||
| ! open a file to write char variables to | ||
| fileHandle = MPAS_io_open(filename, MPAS_IO_WRITE, MPAS_IO_NETCDF4, domain % ioContext, & |
There was a problem hiding this comment.
Since we're not writing any large variables (and so parallel I/O isn't essential), it might increase compatibility if we used MPAS_IO_NETCDF for the format here rather than MPAS_IO_NETCDF4.
There was a problem hiding this comment.
OK. It looks like that parameter isn't used with SMIOL? ncdump -k reports cdf5 when using MPAS_IO_NETCDF4 and when using MPAS_IO_NETCDF4.
There was a problem hiding this comment.
Right -- SMIOL ignores ioformat at present, but the PIO library does make use of ioformat when creating new files.
1. change declaration of constant 2-D character array to allow initializing strings of different sizes. 2. pass optional args to mpas_log_write from close_file_with_message, even when not present. 3. use format of MPAS_IO_NETCDF when creating test data file.
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).