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

Feature 1855 sonarqube fix strlen & strcpy #1898

Merged
merged 24 commits into from
Aug 31, 2021
Merged

Conversation

hsoh-u
Copy link
Collaborator

@hsoh-u hsoh-u commented Aug 31, 2021

Pull Request Testing

  • Describe testing already performed for these changes:

The behavior should not be changed. SonarQube complains strlen, strcpy, and strncpy as Security Hotspot and make sure them to be safe. strlen & strcpy are replaced to m_strlen & m_strcpy, but the implementation of m_strlen & m_strcpy is not updated yet (need more test). Remaining works are replace strncpy to m_strncpy and enhancing m_strlen, m_strcpy, and m_strncpy.

  • src/basic/vx_util/ascii_table.cc: avoid un-initialized variable and checking minimum lines

  • src/basic/vx_util/command_line.cc: deleted unreachable code

  • src/tools/other/mode_time_domain/mm_engine.cc: avoid two similar for loop

  • met/src/tools/other/pb2nc/pb2nc.cc: checks pbl_level before computing PBL and simplify how to find the variables with valid data (checks the first level only). Another issue (Fix PB2NC to better inventory BUFR input data when processing all variables #1894) for checking multiple levels

  • Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:

Run the unit test and make sure the same output

  • Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [No]

  • Do these changes include sufficient testing updates? [No]

  • Will this PR result in changes to the test suite? [No]

    If yes, describe the new output and/or changes to the existing output:

  • Please complete this pull request review by 08-31-2021.

Pull Request Checklist

See the METplus Workflow for details.

  • Complete the PR definition above.
  • Ensure the PR title matches the feature or bugfix branch name.
  • Define the PR metadata, as permissions allow.
    Select: Reviewer(s)
    Select: Organization level software support Project or Repository level development cycle Project
    Select: Milestone as the version that will include these changes
  • After submitting the PR, select Linked issues with the original issue number.
  • After the PR is approved, merge your changes. If permissions do not allow this, request that the reviewer do the merge.
  • Close the linked issue and delete your feature or bugfix branch from GitHub.

Howard Soh added 23 commits August 30, 2021 22:28
… buffer size for strncpy is decided by the targetbuffer size or the input length, not the existing data at the target buffer. The size of existing data at the target buffer is used to erase the previous data
…eck the first level only. Check pbl_level before provcessing
@hsoh-u hsoh-u added this to the MET 10.1.0 milestone Aug 31, 2021
@@ -432,7 +432,7 @@ int do_id()

{

// column += strlen(yytext);
// column += m_strlen(yytext);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// column += m_strlen(yytext);

Instead of editing commented out code, let's just remove it.

@@ -456,7 +456,7 @@ int do_int()

{

// column += strlen(yytext);
// column += m_strlen(yytext);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// column += m_strlen(yytext);

Instead of editing commented out code, let's just remove it.

@@ -657,7 +658,7 @@ int do_int()

{

// Column += strlen(configtext);
// Column += m_strlen(configtext);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Column += m_strlen(configtext);

@@ -678,7 +679,7 @@ bool do_float()

{

// Column += strlen(configtext);
// Column += m_strlen(configtext);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Column += m_strlen(configtext);

@@ -750,8 +750,6 @@ for (j=0; j<N; ++j) {

}

return ( j );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know the logic here, but did you intend to remove this return?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The line 753 can not be executed because of the if statement above: continue or exit( 1 )

      if ( AllowUnrecognizedSwitches )  continue;
      else {
         mlog << Error << "\nCommandLine::next_option() -> "
              << "unrecognized command-line switch \"" << args[j] << "\"\n\n";
         exit ( 1 );
      }
      return ( j );  <-- not reachable

Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway left a comment

Choose a reason for hiding this comment

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

I scrolled through the edits to the 61 modified files. I see that the vast majority of the changes are replacing string functions with the new 'm_'-prefix versions of them. I did recommend removing commented out code instead of modifying it.

I also did merge the current version of develop into the feature_1855_sobarqube_fix (note the typo in the name) branch and then kicked off a full regression test in kiowa:/d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta2/feature_1855/test_regression.log.

Once that regression test completes without error, I'll approve.

Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway left a comment

Choose a reason for hiding this comment

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

I approve of this PR.

The regression test revealed no diffs when compared against the develop branch, as seen in kiowa:/d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta2/feature_1855/test_regression.log

@hsoh-u hsoh-u merged commit 3486de8 into develop Aug 31, 2021
@hsoh-u hsoh-u changed the title Feature 1855 sobarqube fix strlen & strcpy Feature 1855 sonarqube fix strlen & strcpy Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants