Out of bounds write in w3prosfmd_pdlib.F90

[ukmo-ccbunney]

Pull Request Summary

Bugfix for out-of-bounds write to CCOSA and CSINA in w3profsmd_pdlib.F90

Description

There is an out-of-bounds array bug in the w3profsmd_pdlib.F90 module:

WW3/model/src/w3profsmd_pdlib.F90

Lines 3645 to 3646 in a09335e

	CCOSA = FACX * ECOS
	CSINA = FACX * ESIN

CCOSA and CSINA are defined locally as size NTH, but ECOS and ESIN are defined in w3gdatmd with size MSPEC+MTH (MSPEC = NK * NTH)

This is causing a potential out of bounds error and clobbering memory adjacent to CCOSA/CSINA

I think this is what was intended:

CCOSA = FACX * ECOS(1:NTH)
CSINA = FACX * ESIN(1:NTH)

which takes the direction based cosine/sine values from the first frequency band (values are identical for each freq band).

Some changes are expected to the PDLIB regtests due to the possibility of memory being overwritten.
-->

Issue(s) addressed

• fixes Out of bounds array access in w3profsmd_pdlib.F90 #1012

Commit Message

Bugfix to out of bounds array write in w3profsmd_pdlib.f90

Check list

• Branch is up to date with the authoritative repository (NOAA-EMC) develop branch.
• Checked the checklist for a developer submitting to develop.
• If a version number update is required, checked the updating version number checklist.
• If a new feature was added, a regression test for testing the new feature is added.

Testing

• How were these changes tested? Regtests (of PDLIB variant)
• Are the changes covered by regression tests? (If not, why? Do new tests need to be added?) Yes
• Have the matrix regression tests been run (if yes, please note HPC and compiler)? Yes; GNU compiler, Cray EX HPC
• Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.) Some changes expected.
• Please provide the summary output of matrix.comp (matrix.Diff.txt, matrixCompFull.txt and matrixCompSummary.txt):

Results of PDLIB regtests below. Differences in field outputs are only present in ww3_tp2.21.
Most differences seem to be very small, e.g.:

***
/home/h06/christopher.bunney/WW3/UKMO_WW3/regtests/output/ww3_tp2.21/work_b/ww3.201809_hs.nc_diff.txt
***
65209c65209
<     2.741145, 2.331527, 0.692266, 2.702155, 1.944189, 2.083071, 2.161465,
---
>     2.741145, 2.331527, 0.6922661, 2.702155, 1.944189, 2.083071, 2.161465,

All differences in other regtests are due to solely the netCDF in the bugfix branch not having the "forecast_period" variable...not sure why it is missing. I will investigate.

**********************************************************************
********************* non-identical cases ****************************
**********************************************************************
ww3_tp2.17/./work_b                     (2 files differ)
ww3_tp2.17/./work_c                     (2 files differ)
ww3_tp2.19/./work_1C_a                     (10 files differ)
ww3_tp2.19/./work_1B_a                     (10 files differ)
ww3_tp2.21/./work_b                     (4 files differ)
ww3_tp2.21/./work_mb                     (6 files differ)
ww3_tp2.21/./work_b_metis                     (3 files differ)
ww3_tp2.6/./work_pdlib                     (13 files differ)

**********************************************************************
************************ identical cases *****************************
**********************************************************************
ww3_tp2.17/./work_mb
ww3_tp2.17/./work_mc
ww3_tp2.19/./work_1A_a

[JessicaMeixner-NOAA]

@ukmo-ccbunney I got no differences when I ran this and compared with develop, so I'm going to re-run to see if I messed something up on my end.

[ukmo-ccbunney]

@ukmo-ccbunney I got no differences when I ran this and compared with develop, so I'm going to re-run to see if I messed something up on my end.

It's the sort of memory bug that might behave differently on different compilers/systems...
It might be that you just don't see any differences.

Interestingly for me, the only difference I saw was in ww3_tp2.21, which suspiciously is the same regtest that was giving differences for you in #1010 ....

[JessicaMeixner-NOAA]

Okay, I've run this again, and I get no differences. I'm not surprised we're getting different behavior on different systems and I agree this needs to be fixed. I think I was hoping we'd get a difference which would then resolve the other issues which is why I re-ran. This will get merged here shortly.

[JessicaMeixner-NOAA]

While @ukmo-ccbunney got diffs, I did not see any:

**********************************************************************
********************* non-identical cases ****************************
**********************************************************************
mww3_test_03/./work_PR1_MPI_e                     (1 files differ)
mww3_test_03/./work_PR2_UNO_MPI_d2                     (16 files differ)
mww3_test_03/./work_PR1_MPI_d2                     (13 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2_c                     (15 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2_c                     (16 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2                     (16 files differ)
mww3_test_03/./work_PR2_UQ_MPI_d2                     (16 files differ)
mww3_test_03/./work_PR3_UQ_MPI_e                     (1 files differ)
mww3_test_03/./work_PR3_UNO_MPI_e_c                     (1 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2                     (15 files differ)
ww3_tp2.10/./work_MPI_OMPH                     (6 files differ)
ww3_tp2.16/./work_MPI_OMPH                     (4 files differ)
ww3_ufs1.3/./work_a                     (3 files differ)

matrixCompFull.txt
matrixCompSummary.txt
matrixDiff.txt

[aronroland]

Hi @JessicaMeixner-NOAA, @ukmo-ccbunney,

as for this syntax I doubt it is a bug and there it would be interesting to see the error and the compiler output that u have found @ukmo-ccbunney.

Here is my opinion on this:

When assigning an array of larger size to an array of smaller size, Fortran only assigns as many elements as fit into the smaller array.

So, if you have:

A = B

And A has size NX, it is equivalent to:

A = B(1:NX)

In other words, the assignment operation in Fortran truncates the larger array to the size of the smaller one. This behavior can be useful but also can lead to unintentional data loss if you are not careful about array sizes. Here it was used intentionally so i would not say that this a bug. I do not see also that this is compiler/environment depended it should fit the FORTRAN standard given in e.g. International Organization for Standardization. Information technology -- Programming languages -- Fortran. ISO/IEC 1539-1:2018, 2018. To my best knowledge this is the latest and most complete reference.

[ukmo-ccbunney]

Hi @aronroland

Thanks for your input on this.

This was bought to my attention when I got the following runtime error (GNU compiler):
Fortran runtime error: Array bound mismatch for dimension 1 of array 'ccosa' (36/1836)

To be honest, I was not aware of that the original syntax was considered valid Fortran, so apologies for raising a fix for valid code! Are you happy if the fix is left in? To me the explicit index range makes things a bit clearer.

Interestingly, I did get small differences in the output when I made the fix, which made me think that some sort of memory corruption was occuring, but this could possibly have been a red herring...

[JessicaMeixner-NOAA]

@ukmo-ccbunney I think since you got differences in answers, and the fact that this generated a runtime error, we should keep the update in the code. That being said, we should certainly follow fortran standards and I didn't quite understand @aronroland concern about data loss in this particular case, so perhaps I'm missing something obvious here?

[aronroland]

Hi @JessicaMeixner-NOAA, actually I am quite happy that this happened. Since GNU is saying that this is an "error" i am afraid that I am wrong with what I have said before. I will test that on my side and get back to u.