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

Chem: Prevent seg fault due to numgas==0 #1294

Merged
merged 10 commits into from
Oct 7, 2020

Conversation

lpilz
Copy link
Contributor

@lpilz lpilz commented Sep 24, 2020

TYPE: bug fix

KEYWORDS: chem, drydep, wesely

SOURCE: Lukas Pilz (Heidelberg University)

DESCRIPTION OF CHANGES:
Problem:
A namelist option incompatibility between chem_opt (16) and gas_drydep_opt (1) leads to trying to access a variable
that is not available in memory. This leads to unpredictable behaviors. When choosing a tracer-only chem_opt, numgas
is 0. If gas_drydep_opt is 1 (the default), in the Wesely scheme initialization (dep_init), the field dvj is initialized with
size numgas (here 0). Accessing the variable dvj is an error.

Solution:
A fatal error was added for when the Wesely scheme initialization is called with numgas = 0.

This fix is in chem/chemics_init.F and not share/module_check_a_mundo.F because checking the namelist options
would require some tricky-to-maintain hardcoding.

LIST OF MODIFIED FILES:
chem/chemics_init.F

TESTS CONDUCTED:

  1. Run with chem_opt = 16 and gas_drydep_opt = 1 fails appropriately
  2. Run with chem_opt = 16 and gas_drydep_opt = 0 runs as it should
  3. The jenkins testing is OK.

RELEASE NOTE: For WRF-Chem, a namelist option incompatibility between chem_opt (16) and gas_drydep_opt (1) leads to trying to access a variable that is not available in memory. This leads to unpredictable behaviors. A fatal error was added for when the Wesely scheme initialization is called with numgas = 0.

…_opt namelist options

TYPE: bug fix

KEYWORDS: chem, drydep, wesely

SOURCE: Lukas Pilz (Heidelberg University)

DESCRIPTION OF CHANGES:
Problem:
A namelist option incompatibility between chem_opt (16) and gas_drydep_opt (1) leads to stack smashing.
When choosing a tracer-only chem_opt, numgas is 0.
If gas_drydep_opt is then 1 (as is default), in the Wesely scheme initialization (dep_init), the field dvj is initialized with size numgas (here 0).
This leads to stack smashing upon further accesses.

Solution:
A fatal error was added for when the Wesely scheme initialization is called with numgas = 0.

LIST OF MODIFIED FILES: chem/chemics_init.F

TESTS CONDUCTED:
1. Run with chem_opt = 16 and gas_drydep_opt = 1 fails appropriately
2. Run with chem_opt = 16 and gas_drydep_opt = 0 runs as it should

RELEASE NOTE: Added fatal error for incompatible chem_opt and gas_drydep_opt namelist options.
@lpilz lpilz requested a review from a team as a code owner September 24, 2020 18:37
@lpilz lpilz changed the title Prevent stack smashing caused by incompatible chem_opt and gas_drydep… Prevent stack smashing caused by incompatible namelist options Sep 24, 2020
@davegill
Copy link
Contributor

@lpilz
Lukas,

  1. Can we add a better title? It needs to refer to WRF Chem. Also, is only Wesely dry dep a problem with numgas=0? If so, let's include that focused information in the PR title. Users will want to know if they need this update. The PR title should let them know if this important for their research.

  2. Previously, when the wrong namelist options were set, what happened? Did the model fail? Did the model run to completion, but with bad results?

@jordanschnell
Copy link
Contributor

@davegill I can review/approve this, but what is happening with the jenkins test?

@davegill
Copy link
Contributor

@jordanschnell @dudhia @weiwangncar @kkeene44 @smileMchen @powersjg @liujake

I can review/approve this, but what is happening with the jenkins test?

Jordan,
The automated jenkins testing is broken. Please hold off approving or merging. The testing has been broken for 2+ weeks. I know that this is frustrating.

@lpilz
Copy link
Contributor Author

lpilz commented Sep 29, 2020

Hi @davegill

  1. How about "WRF-Chem: Prevent stack smashing due to numgas==0"
  2. I checked around a bit and found instanciations with numgas in wesely_driver, rc, and HL_init. wesely_driver is called in the case we prevent, dep_init is, rc is only called inside of wesely_driver and as far as I see, HL_init is never called. I'll check some more but for now, I think this covers it.
  3. There were random segfaults in the execution, which is why I started debugging. Sometimes the model even ran through...

@davegill davegill changed the title Prevent stack smashing caused by incompatible namelist options Chem: Prevent seg fault due to numgas==0 Sep 29, 2020
@lpilz
Copy link
Contributor Author

lpilz commented Oct 7, 2020

@davegill @jordanschnell

What's the latest update on this PR? Is it ready to be merged?

@jordanschnell
Copy link
Contributor

@lpilz We were waiting on the automated tests to be back up but it seems the check has passed. @davegill, OK to approve and merge?

@davegill
Copy link
Contributor

davegill commented Oct 7, 2020

@lpilz @jordanschnell
Folks,
Yes, we are all good with this PR.

Copy link
Contributor

@jordanschnell jordanschnell left a comment

Choose a reason for hiding this comment

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

Approved by chem

@jordanschnell jordanschnell merged commit 75e865a into wrf-model:release-v4.2.2 Oct 7, 2020
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.

4 participants