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 1903 sonarqube #1911

Merged
merged 32 commits into from
Sep 14, 2021
Merged

Feature 1903 sonarqube #1911

merged 32 commits into from
Sep 14, 2021

Conversation

hsoh-u
Copy link
Collaborator

@hsoh-u hsoh-u commented Sep 10, 2021

Pull Request Testing

New files:

  • str_wrappers.h & str_wrappers.cc (at vx_log) are separated from string_fxns.h/cc (vx_util) to reduce the dependency
  • enum_to_string.h: extern statements at enum_to_string.cc were moved to the header file

Overall:

  • m_strlen, m_strcpy and m_strncpy are re-implemented
  • strlen, strcpy and strncpy is changed to m_strlen, m_strcpy and m_strncpy
  • Some null pointer 0 is changed to NULL (it should be nullptr, but some old compiler does not support nullptr yet)

The assignment at the second or at the third conidtion for || or && logical could cause a side effect. SO separated the condition with assignments to avoid a potential side effects:

  • my_config_scanner.cc
  • concat_string.cc
  • grib_classes.cc
  • tc_stat_job.cc

File specific updates:

  • code.cc: include statements are moved to top. Removed extern statements and included enum_to_string.h:

  • is_dst.cc: added socnt to dst_info[] variable

  • concat_string.cc : check if the pointer is valid before deleting it.

  • color.cc: initialized r,g, and b variables

  • table_lookup.cc: avoid unreachable code return above if statement returns something nor exit

  • nccf_file.cc: cast unixtime (long long) to int

  • python3_script.cc: renamed a local variable name module (a keyword) to py_module

  • cgraph_main.cc, cgraph_main.h, & plot_mode_field.cc: renamed a function name import (a keyword) to import_image

  • pair_data_ensemble.cc: initialize fcst

  • track_info.cc: avoid a potential negative offset

  • aeronet_handler.cc : removed unused variable offset (setting only but no use after setting a value

  • madis2nc.cc: initialize the variable data

  • pb2nc.cc: allows the truncation for some m_strncpy (the to_str must be different from the from_str)

  • Describe testing already performed for these changes:

Just do the unit test and the same outputs are expected

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

unittest and compare the outputs

  • 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 [Fill in date].

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 26 commits September 8, 2021 00:47
…& str_wrappers.h. Changed 0 to NULL to reset the pointer
@JohnHalleyGotway
Copy link
Collaborator

@hsoh-u have you quantified the number of security hotspots identified by SonarQube before/after these changes? If not, please do so. I think that'd be very useful to keep track of.

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.

@hsoh-u I tested by running a full regression test on kiowa and the unit tests for your feature branch failed. From kiowa:/d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1903/test_unit_feature_1903_sonarqube.log

TEST: gen_vx_mask_DATA_APCP_24        - FAIL -   0.191 sec
WARNING: 
WARNING: regex_apply()  truncated a string mat[i] from "A24" to "(nul)"
WARNING: 
WARNING: 
WARNING: regex_apply()  truncated a string mat[i] from "24" to "(nul)"
WARNING: 
ERROR  : 
ERROR  : timestring_to_sec(const char *) -> empty time string!
ERROR  : 

ERROR: /d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1903/MET-feature_1903_sonarqube/test/perl/unit.pl unit_gen_vx_mask.xml failed.

*** UNIT TESTS FAILED ***

Please fix.

@hsoh-u
Copy link
Collaborator Author

hsoh-u commented Sep 13, 2021

It's fixed (some m_strcpy are replaced with m_strncpy). Please do regression test.

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 re-ran the regression test vs develop on 9/13/2021 and it continues to fail as it did last time. See kiowa: d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1903/test_unit_feature_1903_sonarqube.log

TEST: gen_vx_mask_DATA_APCP_24        - FAIL -   0.119 sec
/d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1903/MET-feature_1903_sonarqube/met/share/met/../../bin/gen_vx_mask \
      /d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1903/MET-feature_1903_sonarqube/met/data/sample_fcst/2005080700/wrfprs_ruc13_24.tm00_G212 \
      /d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1903/MET-feature_1903_sonarqube/met/data/sample_fcst/2005080700/wrfprs_ruc13_24.tm00_G212 \
      /d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1903/MET-feature_1903_sonarqube/test_output/gen_vx_mask/DATA_APCP_24_mask.nc \
      -type data -mask_field 'name="APCP"; level="A24";' -thresh 'ge2.54' -v 2
DEBUG 1: Input Grid:		/d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1903/MET-feature_1903_sonarqube/met/data/sample_fcst/2005080700/wrfprs_ruc13_24.tm00_G212
DEBUG 1: Mask File:		/d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1903/MET-feature_1903_sonarqube/met/data/sample_fcst/2005080700/wrfprs_ruc13_24.tm00_G212
DEBUG 2: Parsed Input Grid:	Lambert Conformal (185 x 129)
WARNING: 
WARNING: regex_apply()  truncated a string mat[i] from "A24" to "(nul)"
WARNING: 
WARNING: 
WARNING: regex_apply()  truncated a string mat[i] from "24" to "(nul)"
WARNING: 
ERROR  : 
ERROR  : timestring_to_sec(const char *) -> empty time string!
ERROR  : 

ERROR: /d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1903/MET-feature_1903_sonarqube/test/perl/unit.pl unit_gen_vx_mask.xml failed.

*** UNIT TESTS FAILED ***

ERROR: Command returned with non-zero status (1): test/bin/unit_test.sh

Please make sure all the unit tests run without error before re-submitting.

@hsoh-u
Copy link
Collaborator Author

hsoh-u commented Sep 14, 2021

I modified m_strcpy to m_strncpy in order to use sizeof(to_str) instead of strlen(from_str). But I changed back to use strlen because the unit test for pull request was failed again. I doubt the pull request got the latest code from the feature branch. The instances of m_strcpy & m_strncpy don't match with mine. Here are the counts of m_strcpy & m_strncpy:

directory: /d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1903:

- m_strcpy: 48
- m_strncpy: 54

grep m_strcpy `find /d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1903/MET-feature_1903_sonarqube/met/src/ -type f -name "*cc"` | wc -l
48
grep m_strncpy `find /d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1903/MET-feature_1903_sonarqube/met/src/ -type f -name "*cc"` | wc -l
54

directory: /d1/personal/hsoh/git/pull_request/MET_feature_1903_sonarqube:
- m_strcpy: 36
- m_strncpy: 66

grep m_strcpy `find /d1/personal/hsoh/git/pull_request/MET_feature_1903_sonarqube/met/src/ -type f -name "*cc"` | wc -l
36
grep m_strncpy `find /d1/personal/hsoh/git/pull_request/MET_feature_1903_sonarqube/met/src/ -type f -name "*cc"` | wc -l
66

Please make sure the number of m_strcpy and m_strncpy after checking out feature_1903_sonarqube branch.

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 these changes. I ran a full regression test in kiowa:/d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1903 for feature_1903_sonarqube versus develop and no differences were flagged. I also ran "make test" and inspected the log messages carefully and found that no warning messages about truncated strings remain.

@hsoh-u hsoh-u merged commit eee1ee7 into develop Sep 14, 2021
@hsoh-u hsoh-u linked an issue Sep 14, 2021 that may be closed by this pull request
21 tasks
@hsoh-u hsoh-u deleted the feature_1903_sonarqube branch January 7, 2022 00:06
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.

Resolve PB2NC string truncation warning messages.
2 participants