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 Fortify findings caused by point2grid changes in #1345. #1509

Closed
21 tasks
JohnHalleyGotway opened this issue Oct 6, 2020 · 4 comments · Fixed by #1526
Closed
21 tasks

Fix Fortify findings caused by point2grid changes in #1345. #1509

JohnHalleyGotway opened this issue Oct 6, 2020 · 4 comments · Fixed by #1526
Assignees
Labels
component: code optimization Code optimization issue reporting: DTC AF METplus Air Force METplus Project requestor: NCAR National Center for Atmospheric Research type: task An actionable item of work
Milestone

Comments

@JohnHalleyGotway
Copy link
Collaborator

JohnHalleyGotway commented Oct 6, 2020

Describe the Task

After feature 1345 was merged into the develop branch, 2 new high and 29 new low Fortify findings appeared in the nightly run of Fortify on the develop branch.

This task is to refine the code to make those findings go away.

I have attached PDF's of the Fortify findings from 20201004 and 20201006. The corresponding FPR and MBS files can be found on dakota:
/d3/projects/MET/MET_regression/develop/SAVE-NB20201004/fortify_sca
/d3/projects/MET/MET_regression/develop/SAVE-NB20201006/fortify_sca

There were additional increases for Fortify noted on kiowa for the develop branch. Also fix these.

Foritfy Counts for develop on 20201014: Critical=0 High=353 Medium=0 Low=1053
Fortify Counts for develop on 20201015: Critical=0 High=355 Medium=0 Low=1068
/d1/projects/MET/MET_regression/MET-develop/scripts/fortify/run_nightly.sh: The Fortify counts have CHANGED in NB20201015.

Time Estimate

Estimate the amount of work required here.
Issues should represent approximately 1 to 3 days of work.

Sub-Issues

Consider breaking the task down into sub-issues.

  • Add a checkbox for each sub-issue here.

Relevant Deadlines

List relevant project deadlines here or state NONE.

Funding Source

Define the source of funding and account keys here or state NONE.

Define the Metadata

Assignee

  • Select engineer(s) or no engineer required
  • Select scientist(s) or no scientist required

Labels

  • Select component(s)
  • Select priority
  • Select requestor(s)

Projects and Milestone

  • Review projects and select relevant Repository and Organization ones or add "alert:NEED PROJECT ASSIGNMENT" label
  • Select milestone to next major version milestone or "Future Versions"

Define Related Issue(s)

Consider the impact to the other METplus components.

Task Checklist

See the METplus Workflow for details.

  • Complete the issue definition above, including the Time Estimate and Funding Source.
  • Fork this repository or create a branch of develop.
    Branch name: feature_<Issue Number>_<Description>
  • Complete the development and test your changes.
  • Add/update log messages for easier debugging.
  • Add/update unit tests.
  • Add/update documentation.
  • Push local changes to GitHub.
  • Submit a pull request to merge into develop.
    Pull request: feature <Issue Number> <Description>
  • Define the pull request metadata, as permissions allow.
    Select: Reviewer(s), Project(s), Milestone, and Linked issues
  • Iterate until the reviewer(s) accept and merge your changes.
  • Delete your fork or branch.
  • Close this issue.
@JohnHalleyGotway JohnHalleyGotway added component: fortify type: task An actionable item of work requestor: NCAR National Center for Atmospheric Research labels Oct 6, 2020
@JohnHalleyGotway JohnHalleyGotway added this to the MET 10.0 milestone Oct 6, 2020
@JohnHalleyGotway JohnHalleyGotway added this to To do in MET-10.0.0-beta1 (10/22/20) via automation Oct 6, 2020
@hsoh-u
Copy link
Collaborator

hsoh-u commented Oct 6, 2020

Two PDF files show the number of new high and low findings but not where (filename and line number). I'm not sure how many findings are from nc_utils.cc. I started #1492 already and some refactoring were done at nc_utils.cc which could affect the fortify result. I will working this after #1492. It's not good practice but some of fortify issues will be fixed with #1492.

hsoh-u added a commit that referenced this issue Oct 16, 2020
hsoh-u added a commit that referenced this issue Oct 16, 2020
hsoh-u added a commit that referenced this issue Oct 16, 2020
hsoh-u added a commit that referenced this issue Oct 16, 2020
hsoh-u added a commit that referenced this issue Oct 16, 2020
hsoh-u added a commit that referenced this issue Oct 16, 2020
hsoh-u added a commit that referenced this issue Oct 16, 2020
@hsoh-u
Copy link
Collaborator

hsoh-u commented Oct 16, 2020

One high on point2grid

      for (int xIdx=0; xIdx<from_lon_cnt; xIdx++) {
         for (int yIdx=0; yIdx<from_lat_cnt; yIdx++) {
            int offset = fr_dp.two_to_one(xIdx,yIdx);
            from_data[offset] = fr_dp.get(xIdx,yIdx);   //  <== buffer overflow
         }
      }

Please let me know if this should be corrected. There are more instances of this case.

@hsoh-u hsoh-u linked a pull request Oct 17, 2020 that will close this issue
8 tasks
@hsoh-u hsoh-u moved this from To do to Pull request review in MET-10.0.0-beta1 (10/22/20) Oct 17, 2020
JohnHalleyGotway pushed a commit that referenced this issue Oct 20, 2020
…1526)

* #1509 Corrected argument names

* #1509 Removed unused variable

* #1509 Simplify the macro

* #1509 Release the aloocated memory

* #1509 Remoed unused variables

* #1509 Remoed unused variables

* #1509 Removed the duplicated checking
@JohnHalleyGotway
Copy link
Collaborator Author

JohnHalleyGotway commented Oct 20, 2020

On 20201020, the counts successfully decreased, as described below:

Fortify Counts for develop on 20201019: Critical=0 High=355 Medium=0 Low=1068
Fortify Counts for develop on 20201020: Critical=0 High=351 Medium=0 Low=1037
/d1/projects/MET/MET_regression/MET-develop/scripts/fortify/run_nightly.sh: The Fortify counts have CHANGED in NB20201020.

MET-10.0.0-beta1 (10/22/20) automation moved this from Pull request review to Done Oct 20, 2020
@JohnHalleyGotway
Copy link
Collaborator Author

Howard, a compilation problem popped up via the automated Docker build for the develop branch after merging these changes. Specifically on lines 1845 and 1882 pb2nc.cc:

1845             for(float *tmp_pqtzuv : pqtzuv_list) delete tmp_pqtzuv;
1882       for(float *tmp_pqtzuv : pqtzuv_list) delete tmp_pqtzuv;

The version of the compiler used by the container doesn't like that syntax. Since we want to make MET compile on as many compiler families and versions possible, I'd recommend fixing this issue. Here's the compiler error:

pb2nc.cc: In function 'void process_pbfile(int)':
pb2nc.cc:1845:37: error: range-based 'for' loops are not allowed in C++98 mode
             for(float *tmp_pqtzuv : pqtzuv_list) delete tmp_pqtzuv;
                                     ^
pb2nc.cc:1882:31: error: range-based 'for' loops are not allowed in C++98 mode
       for(float *tmp_pqtzuv : pqtzuv_list) delete tmp_pqtzuv;

MET-10.0.0-beta1 (10/22/20) automation moved this from Done to In progress Oct 20, 2020
hsoh-u added a commit that referenced this issue Oct 21, 2020
JohnHalleyGotway added a commit that referenced this issue Oct 21, 2020
* #1509 Not using new for syntax

* Had to fix the syntax in one more spot.

Co-authored-by: johnhg <johnhg@ucar.edu>
MET-10.0.0-beta1 (10/22/20) automation moved this from In progress to Done Oct 22, 2020
@TaraJensen TaraJensen added the reporting: DTC AF METplus Air Force METplus Project label Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: code optimization Code optimization issue reporting: DTC AF METplus Air Force METplus Project requestor: NCAR National Center for Atmospheric Research type: task An actionable item of work
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants