Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory corruption in nitrate component #27

Merged
merged 8 commits into from
Mar 5, 2021

Conversation

rmontuoro
Copy link
Contributor

@rmontuoro rmontuoro commented Mar 2, 2021

This PR introduces changes to prevent memory corruption occurring in GOCART's nitrate component when diagnostic variables for settling (NISD) and wet removal (NH3WT, NH4WT) are enabled.

The memory corruption was traced back to allocated arrays being passed to subroutines expecting a pointer array argument (see example below):

-    allocate(fluxout, mold=lwi, __STAT__)
 !  Nitrate bin 1 - settles like ammonium sulfate (rhflag = 3)
     rhflag = 3
-    fluxout = 0.
+    nullify(flux_ptr)
+    if (associated(NISD)) flux_ptr => NISD(:,:,1)
     call Chem_SettlingSimpleOrig (self%km, self%klid, rhFlag, MAPL_GRAV, self%cdt, &
                                   1.e-6*self%radius(nNO3an1), self%rhop(nNO3an1), &
-                                  NO3an1, t, airdens, rh2, delp, zle, fluxout, rc)
-    if (associated(NISD)) NISD(:,:,1) = fluxout
+                                  NO3an1, t, airdens, rh2, delp, zle, flux_ptr, rc)
   subroutine Chem_SettlingSimpleOrig ( km, klid, flag, grav, cdt, radiusInp, rhopInp, &
                                        int_qa, tmpu, rhoa, rh, delp, hghte, &
                                        fluxout, rc, vsettleOut, correctionMaring )
   ...
! !OUTPUT PARAMETERS:
   real, pointer, dimension(:,:), intent(inout)  :: fluxout

Additionally, a minor change is included to set the correct intent for a dummy argument.

This PR is contingent on approval of PR #24.

@rmontuoro rmontuoro added Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs bug Something isn't working labels Mar 2, 2021
@rmontuoro rmontuoro self-assigned this Mar 2, 2021
@rmontuoro rmontuoro requested review from a team as code owners March 2, 2021 04:51
tclune
tclune previously approved these changes Mar 2, 2021
Copy link
Contributor

@tclune tclune left a comment

Choose a reason for hiding this comment

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

It would be nice to have a more complete description of the bug that was fixed. I stared a bit at the code, but did not see what was actually wrong with the additional version. E.g., did the changes juts "move the symptoms" of some issue somewhere else?

But I don't want the explanation to get in the way of the merge.

@rmontuoro
Copy link
Contributor Author

It would be nice to have a more complete description of the bug that was fixed. I stared a bit at the code, but did not see what was actually wrong with the additional version. E.g., did the changes juts "move the symptoms" of some issue somewhere else?

But I don't want the explanation to get in the way of the merge.

Some text was missing from my early description. I've added it back along with further details.

@tclune
Copy link
Contributor

tclune commented Mar 2, 2021

@rmontuoro Thanks. I'm a bit surprised that this issue was not diagnosed by the compiler. Might be time to ask Elliot to include occasional builds with the backup compiler which should find more of these.

I the fix might have been simpler. There is no reason for the dummy arguments in the process library to be pointers. That is an artifact of a poor translation from the original F77 interfaces. We have fixed some of those but a more pervasive change is called for.

@mathomp4
Copy link
Member

mathomp4 commented Mar 2, 2021

This has a lot of file changes... should it?

@WilliamJamieson
Copy link
Contributor

This has a lot of file changes... should it?

It has both PR #23 and #24 merged into it in addition to the changes it makes.

@rmontuoro
Copy link
Contributor Author

This has a lot of file changes... should it?

@mathomp4 - As @WilliamJamieson pointed out, this PR also includes #24, which brings in a large number of changes. Hope this is OK, but I can resubmit a new PR with more focused changes.

@tclune
Copy link
Contributor

tclune commented Mar 3, 2021

I think we can take this PR as is, but going forward please try to isolate bug fixes as much as possible. We're a bit sloppy but we tend to take a closer look at understanding bug fixes. Reviews for non-fixes are quite variable in terms of thoroughness and tend to be more about style and extensibility.

@mathomp4
Copy link
Member

mathomp4 commented Mar 3, 2021

I think we can take this PR as is, but going forward please try to isolate bug fixes as much as possible. We're a bit sloppy but we tend to take a closer look at understanding bug fixes. Reviews for non-fixes are quite variable in terms of thoroughness and tend to be more about style and extensibility.

Almost. @rmontuoro would need to add his "if not Baselibs" update from #24 I think.

@WilliamJamieson WilliamJamieson removed the Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs label Mar 5, 2021
gmao-esherman
gmao-esherman previously approved these changes Mar 5, 2021
Copy link
Contributor

@gmao-esherman gmao-esherman left a comment

Choose a reason for hiding this comment

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

I have verified that these changes are zero-diff with the feature/esherman/gocart2g_develop_WilliamUpdate build

Copy link
Contributor

@WilliamJamieson WilliamJamieson left a comment

Choose a reason for hiding this comment

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

My and Elliot's tests show that this is zero-diff.

@tclune
Copy link
Contributor

tclune commented Mar 5, 2021

Using super powers again.

@tclune tclune merged commit 92971a7 into develop Mar 5, 2021
@tclune tclune deleted the bugfix/rmontuoro/fix-mem-corruption branch March 5, 2021 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants